fuzz-introspector: the reachability analysis can be misleading if gold/LTO flags aren't passed properly #7593

evverx · 2022-04-20T19:01:14Z

Looking at https://storage.googleapis.com/oss-fuzz-introspector/systemd/inspector-report/20220419/fuzz_report.html#functions_cov_hit_9 where there are 1238 covered functions and 24 reachable functions I wonder how it works?

My understanding (based on the footnote) is that reachable functions are determined statically and because of that the numbers aren't supposed to match but if they are that far apart I'm not sure how they can be used to figure out what's reachable (along with blockers and so on). I also wonder where 79.17% comes from?

evverx · 2022-04-20T19:11:35Z

I also wonder where 79.17% comes from?

That was easy. It's 19 / 24 * 100

DavidKorczynski · 2022-04-20T19:11:39Z

1238 = functions with coverage from the coverage reports
24 = the number of functions in the statically extracted CFG
5 = the size of the intersection of the above two sets
79% ~= 19 / 24 * 100

~~in this case the static analysis fails to extract the CFG properly -- is there use of indirect pointers?~~ See below comment instead

DavidKorczynski · 2022-04-20T19:15:00Z

I wonder if something different happened here: if the core was compiled without LTO and thus not included in the analysis, this kind of matches the issues with meson, i.e. that gold was actually never used?

Looking at the calltree, there's no depth to any of the calls, so there is a ton of content missing, which should be analysable

https://storage.googleapis.com/oss-fuzz-introspector/systemd/inspector-report/20220419/calltree_view_9.html?scrollToNode=00024

I don't see a reason e.g. config_parse has no more data, but it makes sense it won't if the systemd core was compiled without LTO support and thus no fuzz-intropector plugin ran.

Searching for a given systemd function in the functino table (https://storage.googleapis.com/oss-fuzz-introspector/systemd/inspector-report/20220419/fuzz_report.html#Project-functions-overview) e.g. hashmap_put_stats_by_path I am unable to find it (https://github.com/systemd/systemd/blob/492f9e74ff2b7b07d0999a7241c16ed5e767fdeb/src/shared/conf-parser.c#L447), which confirms the reasoning of fuzz-introspector not running on the given code.

(Logs would help here tbh and I have an issue open on fuzz-introspector for this ossf/fuzz-introspector#176)

evverx · 2022-04-20T19:22:52Z

@DavidKorczynski I think systemd was probably built without lto and I suspect gold wasn't used there either so I'd put the question on hold until systemd is built properly. I have another question though. Why was it built and reported in the first place? I think fuzz-introspector should have failed to somehow signal that it simply can't handle that.

DavidKorczynski · 2022-04-20T19:24:11Z

Why was it built and reported in the first place? I think fuzz-introspector should have failed to somehow signal that it simply can't handle that.

Could you clarify a bit? What is it that it should have detected?

evverx · 2022-04-20T19:27:41Z

What I was trying to say is that for example ./infra/helper.py can be run with build_check to make sure that fuzz targets are actually built with the right sanitizers and so on so if they are miscompiled somehow ./infra/helper.py complains loudly and they aren't even run. By analogy with that it would make sense to make sure that "garbage" like that isn't analyzed by fuzz-introspector.

DavidKorczynski · 2022-04-20T19:38:03Z

The build worked in that there no compilation issues. And fuzz-introspector did analyse a lot of functions in that 4800 functions are included in the report. So from a fuzz-introspector perspective the process went well -- I don't think it makes sense to include heuristics in fuzz-introspector about whether it "analysed the right code". It also seems that some fuzzers has more accurate data? https://storage.googleapis.com/oss-fuzz-introspector/systemd/inspector-report/20220419/fuzz_report.html#Fuzzer:-fuzz-journald-kmsg

In essence this is similar in analogy to builds working finely if some code is sanitized despite there potentially not being a lot of code sanitized -- building will still work.

What makes sense from my perspective here is to include a warning in the introspector report if there is a discrepancy between covered functions and reachable warnings. Another option could be to check if there are functions that are included in the coverage reports but not in the introspector static analysis component -- then we could also show a warning as it's likely the coverage reports and the fuzz-introspector static analysis component were compiled in different ways (i.e. not all fuzz-introspector static analysis were used for all code that received coverage)

evverx · 2022-04-20T19:56:46Z

And fuzz-introspector did analyse a lot of functions in that 4800 functions are included in the report

I have to admit I'm not sure what exactly it analyzed given that neither LTO nor gold was used.

if some code is sanitized despite there potentially not being a lot of code sanitized -- building will still work.

I don't think it will. There are thresholds in the build check that prevent "bogus" fuzz targets from being ever run.

What makes sense from my perspective here is to include a warning in the introspector report if there is a discrepancy between covered functions and reachable warnings

This can't be automated unfortunately. Waiting for warnings to pop up on OSS-Fuzz somewhere can't help me to catch for example meson regressions when it's updated by Dependabot on GitHub and the CI tries to make sure that it doesn't break anything.

DavidKorczynski · 2022-04-20T20:21:15Z

I have to admit I'm not sure what exactly it analyzed given that neither LTO nor gold was used.

Doing some quick analysis on the report I could extract around 370 files (give or take -- it was a bit ad hoc scripting). that were analysed from the folders:

/src/systemd/src/nspawn
/src/systemd/src/udev/net
/src/systemd/src/udev/fido_id
/src/systemd/src/network/netdev
/src/systemd/src/libsystemd-network
/src/systemd/src/boot/efi
/src/systemd/src/network/tc
/src/systemd/src/systemctl
/src/systemd/src/network
/src/systemd/src/resolve
/src/systemd/src/journal-remote
/src/systemd/src/xdg-autostart-generator
/src/systemd/src/udev
/src/systemd/src/fuzz
/src/systemd/src/journal

I don't think it will. There are thresholds in the build check that prevent "bogus" fuzz targets from being ever run.

I think the threshold is 50 edges or something if I remember correctly, so arguable it has some but certainly doesn't ensure all code that should be sanitized is indeed sanitized.

This can't be automated unfortunately. Waiting for warnings to pop up on OSS-Fuzz somewhere can't help me to catch for example meson regressions when it's updated by Dependabot on GitHub and the CI tries to make sure that it doesn't break anything.

Got it, I get where the question is coming from now. I think both should be added then -- for the visuals if a discrepancy happens and also something that can be used in the CI.

I think it makes sense to deploy something similar to the minimum check that OSS-Fuzz does, i.e. we could deploy a check that looks at whether ~50 basic blocks (or whichever number OSS-Fuzz uses) are involved in the graph and if not provide an exit code that the CI can use.

With that said, if you're considering this to be used in the OSS-Fuzz CI then I'm not sure if/when OSS-Fuzz will use a fuzz-introspector check in its CI -- @Navidem may have some input here.

evverx · 2022-04-20T22:12:21Z

As far as I understand systemd started compiling without gold and lto when LDFLAGS were removed so I took a look at another report that was based on the correct data. According to https://oss-fuzz-build-logs.storage.googleapis.com/log-6a04d4cc-752e-45ed-8479-1f6bd6924427.txt -flto was passed to both the compiler and linker and -fuse-ld=gold was passed to switch to gold. The analysis result didn't change: https://storage.googleapis.com/oss-fuzz-introspector/systemd/inspector-report/20220413/fuzz_report.html#functions_cov_hit_10. My guess would be that the shared libraries used by almost all the fuzz targets the systemd project puts in $OUT with

install -Dt "$OUT/src/shared/" \
        "$build"/src/shared/libsystemd-shared-*.so \
        "$build"/src/core/libsystemd-core-*.so

somehow confuse fuzz-introspector.

evverx · 2022-04-20T22:36:13Z

FWIW it confuses CIFuzz as well: #7011

evverx · 2022-04-21T10:07:49Z

@oliverchang on April 13 both gold and LTO were used so I think the reachability analysis isn't compatible with shared libraries put in $OUT. I'll open another issue.

Display visual warning in the event that covered funtions (at runtime, from coverage analysis) is larger than the functions deemed reachable by the static analysis component. We could insert more warnings other places, i.e. start to enforce a "certainty of analysis" for each analysis. Ref: google/oss-fuzz#7593

evverx · 2022-04-21T18:10:00Z

In the meantime I've just opened systemd/systemd#23158

evverx · 2022-04-21T23:22:58Z

FWIW systemd isn't the only project using shared libraries. For example freeradius puts libfreeradius*.so in $OUT and links its fuzz targets against them and because of that calltrees are "flat": https://storage.googleapis.com/oss-fuzz-introspector/freeradius/inspector-report/20220416/calltree_view_0.html. I think projects like that shouldn't pass "sanity" checks either ~~or at least fuzz-introspector should warn that its lto based analysis isn't fully applicable to projects like that~~ (looks like the second part was addressed in ossf/fuzz-introspector#217)

evverx · 2022-11-30T14:20:29Z

FI prints warnings now. I think this issue can be closed.

Display visual warning in the event that covered funtions (at runtime, from coverage analysis) is larger than the functions deemed reachable by the static analysis component. We could insert more warnings other places, i.e. start to enforce a "certainty of analysis" for each analysis. Ref: google/oss-fuzz#7593

oliverchang changed the title ~~fuzz-introspector: the reachability analysis results~~ fuzz-introspector: the reachability analysis can be misleading if gold/LTO flags aren't passed properly Apr 21, 2022

evverx mentioned this issue Apr 21, 2022

fuzz-introspector: the reachability analysis doesn't seem to be compatible with shared libraries put in $OUT #7598

Closed

DavidKorczynski mentioned this issue Apr 21, 2022

post-process: display visual warning if reachability may be off ossf/fuzz-introspector#217

Merged

Navidem mentioned this issue Apr 21, 2022

Links to incomplete fuzz-introspector reports #7601

Closed

DavidKorczynski mentioned this issue Apr 21, 2022

fuzz-introspector: bump #7605

Merged

DavidKorczynski self-assigned this Apr 22, 2022

evverx closed this as completed Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fuzz-introspector: the reachability analysis can be misleading if gold/LTO flags aren't passed properly #7593

fuzz-introspector: the reachability analysis can be misleading if gold/LTO flags aren't passed properly #7593

evverx commented Apr 20, 2022

evverx commented Apr 20, 2022

DavidKorczynski commented Apr 20, 2022 •

edited

Loading

DavidKorczynski commented Apr 20, 2022 •

edited

Loading

evverx commented Apr 20, 2022

DavidKorczynski commented Apr 20, 2022

evverx commented Apr 20, 2022

DavidKorczynski commented Apr 20, 2022 •

edited

Loading

evverx commented Apr 20, 2022

DavidKorczynski commented Apr 20, 2022

evverx commented Apr 20, 2022 •

edited

Loading

evverx commented Apr 20, 2022

evverx commented Apr 21, 2022 •

edited

Loading

evverx commented Apr 21, 2022

evverx commented Apr 21, 2022 •

edited

Loading

evverx commented Nov 30, 2022

fuzz-introspector: the reachability analysis can be misleading if gold/LTO flags aren't passed properly #7593

fuzz-introspector: the reachability analysis can be misleading if gold/LTO flags aren't passed properly #7593

Comments

evverx commented Apr 20, 2022

evverx commented Apr 20, 2022

DavidKorczynski commented Apr 20, 2022 • edited Loading

DavidKorczynski commented Apr 20, 2022 • edited Loading

evverx commented Apr 20, 2022

DavidKorczynski commented Apr 20, 2022

evverx commented Apr 20, 2022

DavidKorczynski commented Apr 20, 2022 • edited Loading

evverx commented Apr 20, 2022

DavidKorczynski commented Apr 20, 2022

evverx commented Apr 20, 2022 • edited Loading

evverx commented Apr 20, 2022

evverx commented Apr 21, 2022 • edited Loading

evverx commented Apr 21, 2022

evverx commented Apr 21, 2022 • edited Loading

evverx commented Nov 30, 2022

DavidKorczynski commented Apr 20, 2022 •

edited

Loading

DavidKorczynski commented Apr 20, 2022 •

edited

Loading

DavidKorczynski commented Apr 20, 2022 •

edited

Loading

evverx commented Apr 20, 2022 •

edited

Loading

evverx commented Apr 21, 2022 •

edited

Loading

evverx commented Apr 21, 2022 •

edited

Loading