Fix bugs reported by asan #5665

Smjert · 2019-07-23T15:01:29Z

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.

linux-foundation-easycla · 2019-07-25T20:15:28Z

The committers are authorized under a signed CLA.

✅ Stefano Bonicatti (6a2819a, 1b4662a, aa8b5f7, 412a720, 0456fdf, 030821c, 711463e, 0ab9c9f, 7e4662f, 60844f8)

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

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

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

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

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

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

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

Issue highlighted by asan activated in PR osquery#5628 StatusLogLine needs to be initialized before using it. PR: osquery#5665

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

…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 · 2019-07-31T10:53:21Z

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:

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.
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.

theopolis

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

theopolis · 2019-08-13T23:30:15Z

How’s this progressing, any updates?

Smjert · 2019-08-14T09:34:00Z

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.