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

Fix bugs reported by asan #5665

Merged
merged 10 commits into from Aug 16, 2019

Conversation

@Smjert
Copy link
Contributor

commented Jul 23, 2019

Work in progress to fix the bugs reported by Asan, as initially reported by #5628.

Some of them will probably require a refactor of the publisher/subscriber framework though, so we need to decide what to do with them and when to actually enable Asan in the CI builds.

This PR temporarily enable Asan which will be removed when before merging.

@Smjert Smjert changed the title Fix reported asan bugs Fix bugs reported by asan Jul 23, 2019
@communitybridge-easycla

This comment has been minimized.

Copy link

commented Jul 25, 2019

CLA Check
The committers are authorized under a signed CLA.

@Smjert Smjert force-pushed the Smjert:stefano/fix/asan-bugs branch 3 times, most recently from dbcfeb9 to 04ac5d2 Jul 25, 2019
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
Issue highlighted by PR osquery#5628

Do not take a reference of a string which is owned by a temporary,
copy it instead.

PR: osquery#5665
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
Issue highlighted by asan activated in PR osquery#5628

Do not use clear() on a vector inizialized with a fixed size to clear
it of its contents when using it as a char buffer.

PR: osquery#5665
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
Issue highlighted by asan activated in PR osquery#5628

Do not use memcpy to copy strings around.
Also, ifa_name size is not guaranteed to be IFNAMSIZ.

PR: osquery#5665
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
Issue highlighted by asan activated in PR osquery#5628

Reading a 8 byte field from a 4 byte aligned struct needs to be
done with memcpy.

PR: osquery#5665
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
Issue highlighted by asan activated in PR osquery#5628

Do not try to read the destination address of a netmask if such address
is a default route.

PR: osquery#5665
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
Issue highlighted by asan activated in PR osquery#5628

When shifting left or right a byte, that must be positive, so
ensure it is.

Light cleanup of a bugged and unused function.
A deeper look into the table implementation is needed.

PR: osquery#5665
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
Issue highlighted by asan activated in PR osquery#5628

Imprecisions between float -> double -> json -> double -> float
lead to out of range values been saved into a float variable.
Since json has only the notion of doubles as floating point numbers,
it's better to require to use them.

Also forced the json parser to parse floating point numbers
with full precision, otherwise the test testing for precision would fail.

PR: osquery#5665
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
Issue highlighted by asan activated in PR osquery#5628

StatusLogLine needs to be initialized before using it.

PR: osquery#5665
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
Issue highlighted by asan activated in PR osquery#5628

EFI_DEVICE_PATH_PROTOCOL and HARDDRIVE_DEVICE_PATH were using
the wrong alignment/padding, since on disk they are written
with no padding.

PR: osquery#5665
Smjert added a commit to Smjert/osquery that referenced this pull request Jul 30, 2019
…rcularBuffer

Issue highlighted by asan activated in PR osquery#5628

Ensure WrappedMessage has no padding since its members are written
consecutively in the buffer.
Also use memcpy when retrieving a WrappedMessage from a buffer, since
it could be written at a misaligned address.

PR: osquery#5665
@Smjert

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

So, I fixed all the bugs that could be fixed without heavy refactoring.
This means that the undefined-behaviors in the pub/sub will stay.

I think the review of this PR has to be done in two phases:

  1. I'll leave the commit that enable Asan, so that it shows that only the pub/sub problems are the one remaining. Also if any change is requested, we'll see if it introduces issues.
  2. When the PR looks ready, I'll remove the commit that enables Asan, so that the CI gets hopefully all green and it can be merged.

I will open an issue about the undefined behaviors in the pub/sub.

@Smjert Smjert marked this pull request as ready for review Jul 31, 2019
Copy link
Member

left a comment

Thanks for putting in the work and investigation here. I like the plan!

@theopolis

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

How’s this progressing, any updates?

@Smjert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

How’s this progressing, any updates?

The PR for me and for now is "ready"; I just need to remove the commit that enabled Asan, which I'll do in a few.

I'm still focusing on the toolchain and as mentioned previously, the UB in the Pub/Sub needs some refactoring, so that should be done later.

Smjert added 8 commits Jul 11, 2019
Issue highlighted by PR #5628

Do not take a reference of a string which is owned by a temporary,
copy it instead.

PR: #5665
Issue highlighted by asan activated in PR #5628

Do not use clear() on a vector inizialized with a fixed size to clear
it of its contents when using it as a char buffer.

PR: #5665
Issue highlighted by asan activated in PR #5628

Do not use memcpy to copy strings around.
Also, ifa_name size is not guaranteed to be IFNAMSIZ.

PR: #5665
Issue highlighted by asan activated in PR #5628

Reading a 8 byte field from a 4 byte aligned struct needs to be
done with memcpy.

PR: #5665
Issue highlighted by asan activated in PR #5628

Do not try to read the destination address of a netmask if such address
is a default route.

PR: #5665
Issue highlighted by asan activated in PR #5628

When shifting left or right a byte, that must be positive, so
ensure it is.

Light cleanup of a bugged and unused function.
A deeper look into the table implementation is needed.

PR: #5665
Issue highlighted by asan activated in PR #5628

Imprecisions between float -> double -> json -> double -> float
lead to out of range values been saved into a float variable.
Since json has only the notion of doubles as floating point numbers,
it's better to require to use them.

Also forced the json parser to parse floating point numbers
with full precision, otherwise the test testing for precision would fail.

PR: #5665
Issue highlighted by asan activated in PR #5628

StatusLogLine needs to be initialized before using it.

PR: #5665
Smjert added 2 commits Jul 29, 2019
Issue highlighted by asan activated in PR #5628

EFI_DEVICE_PATH_PROTOCOL and HARDDRIVE_DEVICE_PATH were using
the wrong alignment/padding, since on disk they are written
with no padding.

PR: #5665
…rcularBuffer

Issue highlighted by asan activated in PR #5628

Ensure WrappedMessage has no padding since its members are written
consecutively in the buffer.
Also use memcpy when retrieving a WrappedMessage from a buffer, since
it could be written at a misaligned address.

PR: #5665
@Smjert Smjert force-pushed the Smjert:stefano/fix/asan-bugs branch from 04ac5d2 to 60844f8 Aug 14, 2019
@alessandrogario alessandrogario merged commit 4f78848 into osquery:master Aug 16, 2019
13 checks passed
13 checks passed
Stefano Bonicatti Thank you for signing the CLA.
Details
osquery Build #20190814.2 succeeded
Details
osquery (Linux) Linux succeeded
Details
osquery (LinuxBuck Release) LinuxBuck Release succeeded
Details
osquery (LinuxCMake Debug) LinuxCMake Debug succeeded
Details
osquery (LinuxCMake Release) LinuxCMake Release succeeded
Details
osquery (Windows) Windows succeeded
Details
osquery (WindowsBuck Release) WindowsBuck Release succeeded
Details
osquery (WindowsCMake Release) WindowsCMake Release succeeded
Details
osquery (macOS) macOS succeeded
Details
osquery (macOSBuck Release) macOSBuck Release succeeded
Details
osquery (macOSCMake Debug) macOSCMake Debug succeeded
Details
osquery (macOSCMake Release) macOSCMake Release succeeded
Details
alessandrogario added a commit that referenced this pull request Aug 16, 2019
Issue highlighted by PR #5628

Do not take a reference of a string which is owned by a temporary,
copy it instead.

PR: #5665
alessandrogario added a commit that referenced this pull request Aug 16, 2019
Issue highlighted by asan activated in PR #5628

Do not use clear() on a vector inizialized with a fixed size to clear
it of its contents when using it as a char buffer.

PR: #5665
alessandrogario added a commit that referenced this pull request Aug 16, 2019
Issue highlighted by asan activated in PR #5628

Do not use memcpy to copy strings around.
Also, ifa_name size is not guaranteed to be IFNAMSIZ.

PR: #5665
alessandrogario added a commit that referenced this pull request Aug 16, 2019
Issue highlighted by asan activated in PR #5628

Reading a 8 byte field from a 4 byte aligned struct needs to be
done with memcpy.

PR: #5665
alessandrogario added a commit that referenced this pull request Aug 16, 2019
Issue highlighted by asan activated in PR #5628

Do not try to read the destination address of a netmask if such address
is a default route.

PR: #5665
alessandrogario added a commit that referenced this pull request Aug 16, 2019
Issue highlighted by asan activated in PR #5628

When shifting left or right a byte, that must be positive, so
ensure it is.

Light cleanup of a bugged and unused function.
A deeper look into the table implementation is needed.

PR: #5665
alessandrogario added a commit that referenced this pull request Aug 16, 2019
Issue highlighted by asan activated in PR #5628

Imprecisions between float -> double -> json -> double -> float
lead to out of range values been saved into a float variable.
Since json has only the notion of doubles as floating point numbers,
it's better to require to use them.

Also forced the json parser to parse floating point numbers
with full precision, otherwise the test testing for precision would fail.

PR: #5665
alessandrogario added a commit that referenced this pull request Aug 16, 2019
Issue highlighted by asan activated in PR #5628

StatusLogLine needs to be initialized before using it.

PR: #5665
alessandrogario added a commit that referenced this pull request Aug 16, 2019
Issue highlighted by asan activated in PR #5628

EFI_DEVICE_PATH_PROTOCOL and HARDDRIVE_DEVICE_PATH were using
the wrong alignment/padding, since on disk they are written
with no padding.

PR: #5665
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.