systemd: bad build

[Dor1s]

Has been detected after lading #838

https://oss-fuzz-build-logs.storage.googleapis.com/index.html

Step #5: INFO: performing bad build checks for /workspace/out/address/src/shared/libsystemd-shared-238.so.
Step #5: INFO: performing bad build checks for /workspace/out/address/fuzz-dns-packet.
Step #5: INFO: performing bad build checks for /workspace/out/address/fuzz-unit-file.
Step #5: INFO: performing bad build checks for /workspace/out/address/fuzz-dhcp-server.
Step #5: /usr/local/bin/bad_build_check: line 37:    21 Segmentation fault      (core dumped) $FUZZER -runs=$MIN_NUMBER_OF_RUNS &> $FUZZER_OUTPUT
Step #5: /usr/local/bin/bad_build_check: line 58: ((: < 256 : syntax error: operand expected (error token is "< 256 ")
Step #5: Broken fuzz targets (1):
Step #5: libsystemd-shared-238.so:
Step #5: BAD BUILD: /workspace/out/address/src/shared/libsystemd-shared-238.so does not seem to be compiled with ASan.
Step #5: BAD BUILD: the fuzzer seems to have either startup crash or exit.
Step #5: ERROR: 25% of fuzz targets seem to be broken. See the list above for a detailed information.

Not sure why we tried to perform the check for /workspace/out/address/src/shared/libsystemd-shared-238.so, does it have +x access rights?

[Dor1s]

Yes, looks like.

root@6bb6478915bb:/out# ls -l src/shared/libsystemd-shared-238.so 
-rwxr-xr-x 1 root root 9956424 Apr 17 21:35 src/shared/libsystemd-shared-238.so

@titanous, @keszybz could you please not to copy that library into the $OUT/ directory?

[keszybz]

This is a private shared library used by the fuzzers. It needs to be installed:

# ldd /out/fuzz-unit-file|grep shared
	libsystemd-shared-238.so => /out/src/shared/libsystemd-shared-238.so (0x00007f851b15e000)

[Dor1s]

Thanks @keszybz for taking a look. Can you link that library statically?

[Dor1s]

There is one more problem now:

root@d69fb5534074:/out# ./fuzz-unit-file -runs=3
=================================================================
==169==ERROR: AddressSanitizer: odr-violation (0x000000bd58e0):
  [1] size=128 'namespace_flag_map' ../../src/systemd/src/shared/nsflags.c:15:33
  [2] size=128 'namespace_flag_map' ../../src/systemd/src/shared/nsflags.c:15:33
These globals were registered at these points:
  [1]:
    #0 0x448620 in __asan_register_globals.part.11 /src/llvm/projects/compiler-rt/lib/asan/asan_globals.cc:358
    #1 0x7138fb in asan.module_ctor (/out/fuzz-unit-file+0x7138fb)

  [2]:
    #0 0x448620 in __asan_register_globals.part.11 /src/llvm/projects/compiler-rt/lib/asan/asan_globals.cc:358
    #1 0x7f8a97aeb23b in asan.module_ctor (/out/src/shared/libsystemd-shared-238.so+0x2cd23b)

==169==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
SUMMARY: AddressSanitizer: odr-violation: global 'namespace_flag_map' at ../../src/systemd/src/shared/nsflags.c:15:33
==169==ABORTING

[keszybz]

Can you link that library statically?

Not easily. We used to link it statically into our binaries, but now we install ~200 binaries (including tests), so using a private share library gives significant space savings. It would be possible to add another compilation option and conditionalize meson.build files to support static libsystemd-shared, but it'd be additional build code (in an area which is already fairly complicated because of all the configuration choices) which would have to be written and maintained. So unless it's absolutely necessary, I'd prefer to not to have that, even if somebody else was to write the patch.

[keszybz]

==169==ERROR: AddressSanitizer: odr-violation (0x000000bd58e0):
[1] size=128 'namespace_flag_map' ../../src/systemd/src/shared/nsflags.c:15:33
[2] size=128 'namespace_flag_map' ../../src/systemd/src/shared/nsflags.c:15:33

namespace_flag_map is defined in nsflags.c which is included in libsystemd-shared.so. I don't see anything special about this array, there are many others like it in libsystemd-shared.
...
That said, looking at objdump -s -j .rodata build/fuzz-unit-file and objdump -s -j .rodata build/src/shared/libsystemd-shared-238.so shows the contents of that table in both binaries. Fishy.

[keszybz]

I was looking at our linking code and made (what I thought was) an unrelated PR: systemd/systemd#8813. But it seems like it could actually fix this odr-violation. Could you check if it makes a difference?

[Dor1s]

Thanks @keszybz ! I've just kick'ed off the rebuild. Will post an update later.

[Dor1s]

Looks like the build still produces out/address/src/shared/libsystemd-shared-238.so :/

[Dor1s]

Ah, sorry, you meant another error. Yes, odr-violation seems to be fixed now, thanks!

[Dor1s]

Ping, there is still an issue with the shared library.

[keszybz]

Yes, the build produces the static library. Like I wrote (#1330 (comment)), the library is there on purpose. Are you sure that the "bad build check" is working correctly? Maybe it's just confused by the fact that this is a shared library not an executable.

(Would it help if we made the library -x?)

[Dor1s]

Yes, -x likely would help, as all executables in the $OUT/ dir are expected to be fuzz targets.

[evverx]

I was trying to reproduce the issue with infra/helper.py by running the following commands:

infra/helper.py build_image --pull systemd
infra/helper.py build_fuzzers --sanitizer address systemd
infra/helper.py check_build systemd

and I got an error that seems to be different from the error mentioned in the description:

Running: docker run --rm -i --privileged -e FUZZING_ENGINE=libfuzzer -e SANITIZER=address -v /home/vagrant/oss-fuzz/build/out/systemd:/out -t gcr.io/oss-fuzz-base/base-runner test_all
testing libsystemd-shared-238.so
/usr/local/bin/test_all: line 36: SKIP_TEST_TARGET_RUN: unbound variable
Check build failed.

.

SKIP_TEST_TARGET_RUN was removed in 69ffa9b, so apparently the docker images haven't been updated yet. Is there any chance they could be updated?

By the way, would it make sense to mention somewhere in the documentation that check_build exists and can be used to check whether everything is fine or not?

[Dor1s]

I think helper.py should ask you:

Pull latest base images (compiler/runtime)? (y/N): 

and if you reply y, you'll get the fresh images.

[evverx]

I use --pull, which is supposed to make helper.py pull the latest base image without asking anything.

[Dor1s]

I see you use it for build_image command, so it pulls a new builder image, but you also need a base-runner image updated because that's where the checks are being run.

[evverx]

My bad. It seems that I should have run infra/helper.py pull_images. I'll try again. Thank you.

[evverx]

I've reproduced the issue and I can confirm that it is gone after I applied systemd/systemd#8927. I'm not sure why check_build has been stuck in the loop for about half an hour:

  # Do not spawn more processes than the number of CPUs available.
  n_child_proc=$(jobs -p | wc -l)
  while [ "$n_child_proc" -eq "$NPROC" ]; do
    sleep 4
    n_child_proc=$(jobs -p | wc -l)
  done

, but it could probably be looked into later.

I think it would be great to mention in https://github.com/google/oss-fuzz/blob/master/docs/reproducing.md#reproducing-oss-fuzz-issues that apart from build_image, build_fuzzers and reproduce, pull_images and check_build should be run too.

[inferno-chromium]

pushing a new build, lets see if it succeeds.

[inferno-chromium]

yay, green build.

[Dor1s]

Thanks @evverx !

[evverx]

No problem. I figured out why check_build got stuck. It turns out that when jobs -p is used in a script, bash doesn't seem to mark jobs as "J_NOTIFIED", which leads to an infinite loop as soon as the number of jobs that has been run equals the number returned by nproc. It can be reproduced by applying the following patch:

diff --git a/infra/base-images/base-runner/test_all b/infra/base-images/base-runner/test_all
index 6a071c1..f5482d6 100755
--- a/infra/base-images/base-runner/test_all
+++ b/infra/base-images/base-runner/test_all
@@ -22,7 +22,7 @@ ALLOWED_BROKEN_TARGETS_PERCENTAGE=10
 TOTAL_TARGETS_COUNT=0

 # Number of CPUs available, this is needed for running tests in parallel.
-NPROC=$(nproc)
+NPROC=1

 # Directories where bad build check results will be written to.
 VALID_TARGETS_DIR="/tmp/valid_fuzz_targets"

and can be fixed by using jobs -rp instead of jobs -p:

diff --git a/infra/base-images/base-runner/test_all b/infra/base-images/base-runner/test_all
index 6a071c1..c9b6f5e 100755
--- a/infra/base-images/base-runner/test_all
+++ b/infra/base-images/base-runner/test_all
@@ -57,10 +57,10 @@ for FUZZER_BINARY in $(find $OUT/ -executable -type f); do
   TOTAL_TARGETS_COUNT=$[$TOTAL_TARGETS_COUNT+1]

   # Do not spawn more processes than the number of CPUs available.
-  n_child_proc=$(jobs -p | wc -l)
+  n_child_proc=$(jobs -rp | wc -l)
   while [ "$n_child_proc" -eq "$NPROC" ]; do
     sleep 4
-    n_child_proc=$(jobs -p | wc -l)
+    n_child_proc=$(jobs -rp | wc -l)
   done
 done