Closed mrc0mmand closed 1 year ago
dvdhrm
commented
2 years ago The unaligned read is an actual issue, which I now fixed. I don't see how these other issues can actually arise. They all boil down to the sanitizer assuming the constructor failed, when I don't see how that can actually happen.
The libasan and libubsan have been added to the CI images (though I haven't rebuilt the images, yet).
evverx
commented
2 years ago I was going to report that dbus-broker-launcher fails to start under UBSan but it appears both backtraces I extracted from a container where systemd was tested with dfuzzer are here as well.
They all boil down to the sanitizer assuming the constructor failed
As far as I understand UBSan complains that for example null is passed to memcpy(dup, label, n_label) as label. It doesn't complain about dup so the constructor doesn't fail. It's UB in the sense that compilers can assume that pointers passed to memcpy, memset and so on can't be NULLs so they can be safely optimized away. It potentially can lead to crashes. For example, here's what the GCC compiler can do
GCC might now optimize away the null pointer check in code like:
int copy (int* dest, int* src, size_t nbytes) {
memmove (dest, src, nbytes);
if (src != NULL)
return *src;
return 0;
}
The pointers passed to memmove (and similar functions in <string.h>) must be non-null even when nbytes==0, so GCC can use that information to remove the check after the memmove call. Calling copy(p, NULL, 0) can therefore deference a null pointer and crash.
The example above needs to be fixed to avoid the invalid memmove call, for example:
if (nbytes != 0)
memmove (dest, src, nbytes);@evverx Indeed, I misread those errors. I now added c_mem*() functions to c-stdaux, similar to the helpers in systemd src/basic/memory-util.h. I think I converted most call-sites. If anything shows up again, please let me know!
Thanks for the report!
evverx
commented
2 years ago Looks like the issues are gone. Thanks!
My guess would be that @mrc0mmand was planning to start running the unit tests under ASan/UBSan on the CI so I'll leave it to @mrc0mmand to decide whether this issue should be closed or not.
dvdhrm
commented
1 year ago I am closing this. The issues have been fixed and libubsan/libasan is available in the CI images. Thanks to everyone involved! Maybe at one point we can get the integration into the CI runs as well.
Hey!
I gave the tests a quick run with sanitizers and it found couple of issues (both in dbus-broker and the respective c-util subprojects:
In case there would be an interest to run this in a CI periodically, it shouldn't be a much of an issue once
libasanandlibubsanare added to the CI image (https://github.com/c-util/automation/blob/main/src/ci-c-util/Dockerfile).