Make ASAN work with tests mocking libc functions and fix several errors caught by UBSAN #732
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
igaw
reviewed
Oct 31, 2023
The README currently suggests setting LD_PRELOAD to the libasan path. However, this seems to be unnecessary (at least on my gcc 9 setup). The test executables request to link with libasan.so (before libc.so), so it is automatically loaded. The recommended suggestion also ends up running ninja with ASAN instrumentation, which causes it to fail with leaks detected. Signed-off-by: Caleb Sander <[email protected]>
The undefined behavior sanitizer can catch a bunch of common C bugs. Add instructions to the README for enabling it via meson. Signed-off-by: Caleb Sander <[email protected]>
Several tests mock libc functions by setting LD_PRELOAD to a shared library containing mock implementations of the functions. Currently this prevents the tests from running with ASAN. ASAN complains that libasan doesn't come first in LD_PRELOAD and aborts. But in practice this doesn't seem to be an issue. Sample ASAN issues added to libnvme, the test cases, and the mocks are all correctly reported. So suppress this warning by setting the environment variable ASAN_OPTIONS=verify_asan_link_order=0. In case the user does want to inject a specific libasan, change the tests' use of LD_PRELOAD to append the mock shared library rather than overwriting LD_PRELOAD entirely. Signed-off-by: Caleb Sander <[email protected]>
According to the C standard, it is undefined behavior to pass NULL pointers to standard library functions. This includes the mem*() family of functions, even they are passed a length of 0. So avoid calling these functions when the length is 0. Signed-off-by: Caleb Sander <[email protected]>
Avoid casting byte-aligned pointers to pointers with higher alignment. Loading or storing values with higher alignment is undefined behavior, since some processors don't allow unaligned memory accesses and compilers may assume pointers of different types don't alias. Perform an explicit memcpy() in two places, which an optimizing compiler can easily replace with a single load/store on supported architectures. While we're touching this code, also use IN6_IS_ADDR_V4MAPPED() instead of hand-rolling it. Signed-off-by: Caleb Sander <[email protected]>
Avoid casting byte-aligned pointers to pointers with higher alignment. Loading or storing values with higher alignment is undefined behavior, since some processors don't allow unaligned memory accesses and compilers may assume pointers of different types don't alias. Perform an explicit memcpy(), which an optimizing compiler can easily replace with a single load/store on supported architectures. Signed-off-by: Caleb Sander <[email protected]>
Bit shifts that overflow the resulting type are undefined behavior in C. C arithmetic promotes to ints all smaller integer types. There are several places where a 32-bit unsigned value is constructed by shifting a u8 or u16 to the most significant bits. Since this overflows a signed 32-bit integer, explicitly cast to u32 to avoid the UB. Technically, an int is allowed to only be 16 bits, so any shift that could set bit 15 or higher is UB. But platforms where int is s16 are not very common, so it's likely not worth the effort to fix the code. Signed-off-by: Caleb Sander <[email protected]>
Bit shifts that overflow the resulting type are undefined behavior in C. C arithmetic promotes to ints all smaller integer types. There are several places where a 32-bit unsigned value is constructed by shifting a u8 or u16 to the most significant bits. Since this overflows a signed 32-bit integer, explicitly cast to u32 to avoid the UB. Technically, an int is allowed to only be 16 bits, so any shift that could set bit 15 or higher is UB. But platforms where int is s16 are not very common, so it's likely not worth the effort to fix the code. Signed-off-by: Caleb Sander <[email protected]>
Bit shifts that overflow the resulting type are undefined behavior in C. C arithmetic promotes to ints all smaller integer types. There are several places where a 32-bit unsigned value is constructed by shifting a u8 or u16 to the most significant bits. Since this overflows a signed 32-bit integer, explicitly cast to u32 to avoid the UB. Technically, an int is allowed to only be 16 bits, so any shift that could set bit 15 or higher is UB. But platforms where int is s16 are not very common, so it's likely not worth the effort to fix the code. Signed-off-by: Caleb Sander <[email protected]>
|
Just splitted the last two patches, because I think it groups the changes a bit better and thus easier to spot the what's going on. |
|
Thanks! |
matoro
added a commit
to matoro/gentoo
that referenced
this pull request
Dec 16, 2023
See: linux-nvme/libnvme#732 Bug: https://bugs.gentoo.org/908793 Signed-off-by: Matoro Mahri <[email protected]>
matoro
added a commit
to matoro/gentoo
that referenced
this pull request
Dec 16, 2023
See: linux-nvme/libnvme#732 Bug: https://bugs.gentoo.org/908793 Signed-off-by: Matoro Mahri <[email protected]>
gentoo-bot
pushed a commit
to gentoo/gentoo
that referenced
this pull request
Dec 16, 2023
See: linux-nvme/libnvme#732 Bug: https://bugs.gentoo.org/908793 Signed-off-by: Matoro Mahri <[email protected]> Closes: #34304 Signed-off-by: Sam James <[email protected]>
MocioF
pushed a commit
to MocioF/gentoo
that referenced
this pull request
Dec 17, 2023
See: linux-nvme/libnvme#732 Bug: https://bugs.gentoo.org/908793 Signed-off-by: Matoro Mahri <[email protected]> Closes: gentoo#34304 Signed-off-by: Sam James <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing out ASAN revealed that the new tests using
LD_PRELOADwith a shared library containing mocks for libc functions cause ASAN to abort at startup. Suppress this check because ASAN seems to have no problems catching memory issues inlibnvme, the tests, or the mocks.Also fix several issues caught by enabling UBSAN in the unit tests:
memcmp()andmemset()intvalues that should beu32s