google / oss-fuzz

OSS-Fuzz - continuous fuzzing for open source software.
https://google.github.io/oss-fuzz
Apache License 2.0
10.49k stars 2.22k forks source link

CIFuzz can't detect affected fuzz targets #7011

Closed evverx closed 1 year ago

evverx commented 2 years ago

I've just opened https://github.com/systemd/systemd/pull/21755 (where I changed a fuzz target to make sure the "upload" action can upload the crash). Looking at the logs I think that CIFuzz should trim the "/work/build/../../" build prefix to be able to detect affected fuzzers:

2021-12-13 10:39:11,938 - root - INFO - Files changed in PR: ['.github/workflows/cifuzz.yml', 'src/fuzz/fuzz-catalog.c']
...
2021-12-13 10:39:12,800 - urllib3.connectionpool - DEBUG - https://storage.googleapis.com:443 "GET /oss-fuzz-coverage/systemd/fuzzer_stats/20211213/fuzz-catalog.json HTTP/1.1" 200 187432
2021-12-13 10:39:12,811 - root - INFO - Fuzz target fuzz-catalog is affected by: ['/work/build/../../src/systemd/src/basic/alloc-util.c', '/work/build/../../src/systemd/src/basic/alloc-util.h', '/work/build/../../src/systemd/src/basic/env-util.c', '/work/build/../../src/systemd/src/basic/errno-util.h', '/work/build/../../src/systemd/src/basic/fd-util.c', '/work/build/../../src/systemd/src/basic/fd-util.h', '/work/build/../../src/systemd/src/basic/fileio.c', '/work/build/../../src/systemd/src/basic/fileio.h', '/work/build/../../src/systemd/src/basic/fs-util.c', '/work/build/../../src/systemd/src/basic/hashmap.c', '/work/build/../../src/systemd/src/basic/hashmap.h', '/work/build/../../src/systemd/src/basic/hexdecoct.c', '/work/build/../../src/systemd/src/basic/log.c', '/work/build/../../src/systemd/src/basic/macro.h', '/work/build/../../src/systemd/src/basic/memory-util.c', '/work/build/../../src/systemd/src/basic/memory-util.h', '/work/build/../../src/systemd/src/basic/mempool.c', '/work/build/../../src/system...
2021-12-13 10:39:12,811 - root - INFO - Fuzz target fuzz-catalog is not affected
...
2021-12-13 10:39:14,487 - root - INFO - No affected fuzz targets detected, keeping all as fallback.
evverx commented 2 years ago

The logs can be found at https://github.com/systemd/systemd/runs/4504761165?check_suite_focus=true. Now that the job is finished I can see that /work/build/../../src/systemd/src/fuzz/fuzz-catalog.c is on the list of the files affecting the fuzz target:

2021-12-13T10:39:12.8126405Z 2021-12-13 10:39:12,811 - root - INFO - Fuzz target fuzz-catalog is affected by: ['/work/build/../../src/systemd/src/basic/alloc-util.c', '/work/build/../../src/systemd/src/basic/alloc-util.h', '/work/build/../../src/systemd/src/basic/env-util.c', '/work/build/../../src/systemd/src/basic/errno-util.h', '/work/build/../../src/systemd/src/basic/fd-util.c', '/work/build/../../src/systemd/src/basic/fd-util.h', '/work/build/../../src/systemd/src/basic/fileio.c', '/work/build/../../src/systemd/src/basic/fileio.h', '/work/build/../../src/systemd/src/basic/fs-util.c', '/work/build/../../src/systemd/src/basic/hashmap.c', '/work/build/../../src/systemd/src/basic/hashmap.h', '/work/build/../../src/systemd/src/basic/hexdecoct.c', '/work/build/../../src/systemd/src/basic/log.c', '/work/build/../../src/systemd/src/basic/macro.h', '/work/build/../../src/systemd/src/basic/memory-util.c', '/work/build/../../src/systemd/src/basic/memory-util.h', '/work/build/../../src/systemd/src/basic/mempool.c', '/work/build/../../src/systemd/src/basic/missing_syscall.h', '/work/build/../../src/systemd/src/basic/process-util.c', '/work/build/../../src/systemd/src/basic/random-util.c', '/work/build/../../src/systemd/src/basic/siphash24.c', '/work/build/../../src/systemd/src/basic/siphash24.h', '/work/build/../../src/systemd/src/basic/string-util.c', '/work/build/../../src/systemd/src/basic/string-util.h', '/work/build/../../src/systemd/src/basic/tmpfile-util.c', '/work/build/../../src/systemd/src/basic/umask-util.h', '/work/build/../../src/systemd/src/basic/unaligned.h', '/work/build/../../src/systemd/src/basic/util.h', '/work/build/../../src/systemd/src/fundamental/macro-fundamental.h', '/work/build/../../src/systemd/src/fundamental/string-util-fundamental.c', '/work/build/../../src/systemd/src/fundamental/string-util-fundamental.h', '/work/build/../../src/systemd/src/fuzz/fuzz-catalog.c', '/work/build/../../src/systemd/src/libsystemd/sd-id128/sd-id128.c', '/work/build/../../src/systemd/src/libsystemd/sd-journal/catalog.c']
2021-12-13T10:39:12.8135642Z 2021-12-13 10:39:12,811 - root - INFO - Fuzz target fuzz-catalog is not affected.
evverx commented 2 years ago

Looks like something like

diff --git a/infra/cifuzz/affected_fuzz_targets.py b/infra/cifuzz/affected_fuzz_targets.py
index be35c5cc..aa54177d 100644
--- a/infra/cifuzz/affected_fuzz_targets.py
+++ b/infra/cifuzz/affected_fuzz_targets.py
@@ -91,9 +91,10 @@ def is_fuzz_target_affected(coverage, fuzz_target_path, files_changed):

   logging.info('Fuzz target %s is affected by: %s', fuzz_target, covered_files)
   for filename in files_changed:
-    if filename in covered_files:
-      logging.info('Fuzz target %s is affected by changed file: %s',
-                   fuzz_target, filename)
+    for covered_file in covered_files:
+      if covered_file.endswith(filename):
+        logging.info('Fuzz target %s is affected by changed file: %s',
+                     fuzz_target, filename)
       return True

   logging.info('Fuzz target %s is not affected.', fuzz_target)

should fix it. I haven't tested it though

evverx commented 2 years ago

I'm not sure where "/work/build/../.." comes from though. Having taken a look at lxc (with "src/lxc/../..") and zstd (with "tests/fuzz/../..", I think it's probably a clang regression. As far as I can remember at some point CIFuzz worked correctly without having to trim those prefixes.

oliverchang commented 2 years ago

@jonathanmetzman could you PTAL?

jonathanmetzman commented 2 years ago

Sure.

jonathanmetzman commented 2 years ago

Gonna make covered_files absolute paths to solve this.

evverx commented 2 years ago

@jonathanmetzman I wonder if it would be possible to reopen this issue? From https://github.com/google/oss-fuzz/pull/7014#issuecomment-994100783

Files changed in PR: ['.github/workflows/cifuzz.yml', 'src/fuzz/fuzz-catalog.c']
...
2021-12-14 22:18:49,203 - urllib3.connectionpool - DEBUG - https://storage.googleapis.com:443 "GET /oss-fuzz-coverage/systemd/fuzzer_stats/20211214/fuzz-catalog.json HTTP/1.1" 200 187432
2021-12-14 22:18:49,258 - root - INFO - Fuzz target fuzz-catalog is affected by: ['/src/systemd/src/basic/alloc-util.c', '/src/systemd/src/basic/alloc-util.h', '/src/systemd/src/basic/env-util.c', '/src/systemd/src/basic/errno-util.h', '/src/systemd/src/basic/fd-util.c', '/src/systemd/src/basic/fd-util.h', '/src/systemd/src/basic/fileio.c', '/src/systemd/src/basic/fileio.h', '/src/systemd/src/basic/fs-util.c', '/src/systemd/src/basic/hashmap.c', '/src/systemd/src/basic/hashmap.h', '/src/systemd/src/basic/hexdecoct.c', '/src/systemd/src/basic/log.c', '/src/systemd/src/basic/macro.h', '/src/systemd/src/basic/memory-util.c', '/src/systemd/src/basic/memory-util.h', '/src/systemd/src/basic/mempool.c', '/src/systemd/src/basic/missing_syscall.h', '/src/systemd/src/basic/process-util.c', '/src/systemd/src/basic/random-util.c', '/src/systemd/src/basic/siphash24.c', '/src/systemd/src/basic/siphash24.h', '/src/systemd/src/basic/string-util.c', '/src/systemd/src/basic/string-util.h', '/src/systemd/src/basic/tmpfile-util....
2021-12-14 22:18:49,258 - root - INFO - Fuzz target fuzz-catalog is not affected.
...
INFO - No affected fuzz targets detected, keeping all as fallback.

As far as I can tell, paths to files changed in PRs are always relative so "/" at the beginning of covered files prevents CIFuzz from matching them.

jonathanmetzman commented 2 years ago

I'm not sure I agree that this is broken in general: see https://github.com/facebook/zstd/runs/4943587481?check_suite_focus=true

Files changed in PR: ['lib/compress/zstd_compress.c', 'lib/compress/zstd_compress_internal.h', 'lib/compress/zstd_cwksp.h', 'tests/regression/results.csv', 'tests/zstreamtest.c']
...
022-01-25T21:34:07.1402967Z 2022-01-25 21:34:07,139 - root - INFO - Fuzz target sequence_compression_api is affected by: ['lib/common/bitstream.h', 'lib/common/cpu.h', 'lib/common/entropy_common.c', 'lib/common/error_private.h', 'lib/common/fse.h', 'lib/common/fse_decompress.c', 'lib/common/mem.h', 'lib/common/xxhash.h', 'lib/common/zstd_common.c', 'lib/common/zstd_internal.h', 'lib/compress/fse_compress.c', 'lib/compress/hist.c', 'lib/compress/huf_compress.c', 'lib/compress/zstd_compress.c', 'lib/compress/zstd_compress_internal.h', 'lib/compress/zstd_compress_literals.c', 'lib/compress/zstd_compress_sequences.c', 'lib/compress/zstd_cwksp.h', 'lib/compress/zstd_double_fast.c', 'lib/compress/zstd_fast.c', 'lib/compress/zstd_lazy.c', 'lib/compress/zstd_ldm.c', 'lib/compress/zstd_opt.c', 'lib/compress/zstdmt_compress.c', 'lib/decompress/huf_decompress.c', 'lib/decompress/zstd_ddict.c', 'lib/decompress/zstd_decompress.c', 'lib/decompress/zstd_decompress_block.c', 'lib/decompress/zstd_decompress_internal.h', 'lib/legacy/zstd_legacy.h', 'tests/fuzz/fuzz_data_producer.c', 'tests/fuzz/fuzz_helpers.c', 'tests/fuzz/sequence_compression_api.c']
2022-01-25T21:34:07.1405030Z 2022-01-25 21:34:07,139 - root - INFO - Fuzz target sequence_compression_api is affected by changed file: lib/compress/zstd_compress.c
evverx commented 2 years ago

I'm pretty sure the paths there were absolute as well when I opened the issue. Anyway, systemd is still broken

2022-01-26T15:42:26.2573470Z 2022-01-26 15:42:26,254 - root - INFO - Files changed in PR: ['src/libsystemd-network/sd-dhcp-server.c']

2022-01-26T15:42:30.8138650Z 2022-01-26 15:42:30,813 - root - INFO - Fuzz target fuzz-dhcp-server is affected by: ['/src/systemd/src/basic/alloc-util.c', '/src/systemd/src/basic/alloc-util.h', '/src/systemd/src/basic/env-util.c', '/src/systemd/src/basic/errno-util.h', '/src/systemd/src/basic/fd-util.c', '/src/systemd/src/basic/fd-util.h', '/src/systemd/src/basic/hash-funcs.c', '/src/systemd/src/basic/hash-funcs.h', '/src/systemd/src/basic/hashmap.c', '/src/systemd/src/basic/hashmap.h', '/src/systemd/src/basic/in-addr-util.c', '/src/systemd/src/basic/log.c', '/src/systemd/src/basic/macro.h', '/src/systemd/src/basic/memory-util.c', '/src/systemd/src/basic/memory-util.h', '/src/systemd/src/basic/mempool.c', '/src/systemd/src/basic/missing_syscall.h', '/src/systemd/src/basic/ordered-set.h', '/src/systemd/src/basic/prioq.c', '/src/systemd/src/basic/process-util.c', '/src/systemd/src/basic/random-util.c', '/src/systemd/src/basic/set.h', '/src/systemd/src/basic/siphash24.c', '/src/systemd/src/basic/sparse-endian.h', '/src/systemd/src/basic/time-util.c', '/src/systemd/src/basic/time-util.h', '/src/systemd/src/basic/unaligned.h', '/src/systemd/src/basic/utf8.c', '/src/systemd/src/fundamental/macro-fundamental.h', '/src/systemd/src/libsystemd-network/dhcp-network.c', '/src/systemd/src/libsystemd-network/dhcp-option.c', '/src/systemd/src/libsystemd-network/dhcp-packet.c', '/src/systemd/src/libsystemd-network/fuzz-dhcp-server.c', '/src/systemd/src/libsystemd-network/sd-dhcp-server.c', '/src/systemd/src/libsystemd/sd-event/sd-event.c', '/src/systemd/src/systemd/_sd-common.h']
2022-01-26T15:42:30.8140663Z 2022-01-26 15:42:30,813 - root - INFO - Fuzz target fuzz-dhcp-server is not affected.

And as far as I can tell https://github.com/google/oss-fuzz/pull/7142 can't fix it :-(

jonathanmetzman commented 2 years ago

I see systemd is still broken. I see the following here:

Files changed in PR: ['src/libsystemd-network/sd-dhcp-server.c']
...
Fuzz target fuzz-journald-audit is affected by: ['/src/systemd/src/basic/alloc-util.c', ...

The problem is that fuzzer-affected by files are absolute while changed files is relative to /src/systemd. I think the best way to fix this is for all paths be absolute. Or alternatively all paths should be relative to /src/systemd.

But I'm kinda mystified why in zstd the fuzzer-affected files are relative (like the diffed ones).

I guess to fix this we can do the following:

  1. make diffed files relative to the directory they are diffed in (i.e. /src/$REPO)
  2. normalize the fuzzer-affected paths and if they are not absolute make them absolute to /src/$REPO.
evverx commented 2 years ago

But I'm kinda mystified why in zstd the fuzzer-affected files are relative (like the diffed ones).

My guess would be that most projects have relative paths in their coverage reports. systemd is different probably because it installs shared libraries to $OUT and that tends to affect coverage reports in mysterious ways like https://github.com/google/oss-fuzz/issues/6524#issuecomment-945046596.

evverx commented 1 year ago

All in all I think it's more of a systemd issue in the sense that it happens because systemd is built with a bunch of kludges making it more or less work on OSS-Fuzz. I think it can (and probably should) be fixed on the systemd side. Closing.