Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

out_cloudwatch: fix integer overflow in timestamps (issue #3640) #3648

Merged
merged 1 commit into from
Aug 5, 2021

Conversation

nbertram
Copy link
Contributor

This is the simplest fix to avoid integer overflows when multiplying (32 bit) timestamp values by 1000 on 32 bit systems (ARMv7). See the related issue for context.

Fixes #3640


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • [See issue] Example configuration file for the change
  • [TODO] Debug log output from testing the change
  • [TODO] Attached Valgrind output that shows no leaks or memory corruption was found

Documentation

  • [N/A] Documentation required for this feature

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@nbertram
Copy link
Contributor Author

By the way, the test suite is already failing on ARMv7, but not for Cloudwatch. There doesn't seem to be a test for the formatting of the log HTTP messages.

93% tests passed, 5 tests failed out of 68

Label Time Summary:
internal    =  34.61 sec*proc (31 tests)
runtime     = 875.80 sec*proc (35 tests)

Total Test time (real) = 918.49 sec

The following tests FAILED:
	  4 - flb-rt-in_proc (Failed)
	 21 - flb-rt-out_elasticsearch (Failed)
	 35 - flb-rt-out_td (Failed)
	 44 - flb-it-network (Failed)
	 63 - flb-it-aws_util (Failed)

@edsiper edsiper requested a review from PettitWesley June 17, 2021 13:05
@edsiper edsiper added AWS Issues with AWS plugins or experienced by users running on AWS and removed docs-required labels Jun 17, 2021
@PettitWesley
Copy link
Contributor

@edsiper can you review this one- cross-compiler/cross platform stuff is completely outside of my expertise.

@nbertram make sure you submit a PR for this same commit (after its approved) against the 1.7 branch as well so that we can get the fix released in the next 1.7 patch.

@PettitWesley PettitWesley requested review from edsiper and removed request for PettitWesley June 20, 2021 01:07
@nokute78
Copy link
Collaborator

nokute78 commented Jun 23, 2021

@nbertram Can you check DCO error ?
https://probot.github.io/apps/dco/

@edsiper @PettitWesley
I confirmed. The patch is looks good to me.

I tested a test code on Ubuntu 20.04(x86_64)

environment setting

sudo apt install g++-arm-linux-gnueabihf
sudo apt-get install qemu-system-arm

test code

timestamp is current implementation.
timestamp_patch is fixed implementation by this PR.

#include <stdio.h>
#include <time.h>

int main() {
  unsigned long long timestamp;
  unsigned long long timestamp_patch;
  time_t sec = 1624437067;
  long nsec = 456789000;


  timestamp = (unsigned long long)(sec * 1000 +
                                   nsec / 1000000);

  timestamp_patch  = (unsigned long long)(sec * 1000ull +
                                          nsec / 1000000);

  printf("timestamp    =%lld\n", timestamp);
  printf("timestamp(p) =%lld\n", timestamp_patch);
}

Output

timestamp(p) is right value.

$ arm-linux-gnueabihf-gcc  -Wall -o a --static a.c
$ ./a
timestamp    =939429568
timestamp(p) =1624437067456

@PettitWesley
Copy link
Contributor

@nbertram Can you please fix the DCO error?

Commit the message like this as an example:

git commit --sign -m "..."

@PettitWesley
Copy link
Contributor

@edsiper What should we do to get this fix merged? Should we take the same changes and re-commit them and attribute them to @nbertram?

@nbertram
Copy link
Contributor Author

nbertram commented Aug 1, 2021

Sorry I'll get on to this shorty. It's got an entry in my todo list now!

@nbertram
Copy link
Contributor Author

nbertram commented Aug 4, 2021

@PettitWesley sorry for the delay, have added the DCO.

Anything else you need from me?

Copy link
Collaborator

@nokute78 nokute78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@nokute78 nokute78 merged commit 4dba97a into fluent:master Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AWS Issues with AWS plugins or experienced by users running on AWS docs-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

out_cloudwatch seems to have an integer overflow problem with timestamps on ARMv7
4 participants