AFL dynamic allocation #11877

xavierleroy · 2023-01-09T10:41:06Z

The friendly bot reminded me to submit this code as a PR.

Instrumented code produced by ocamlopt -afl-instrument but not run under AFL needs a 64 k dummy buffer at run-time. Currently, this buffer is allocated statically, and therefore present in all ocamlopt-generated executables, whether compiled with -afl-instrument or without, whether AFL was selected at configuration-time or not. That's a bit of a waste.

This PR implements dynamic allocation of the AFL buffer when an AFL-instrumented executable starts. It is based on the proposal at #10167 and its discussion. It also un-breaks the build when configured with -with-afl.

The tests in testsuite/tests/afl-instrumentation fail on my machine, both with and without this PR, both on trunk and on 5.0 and on 4.14. So, that's probably a problem with my installation of AFL tools. Yet, if @jmid (the author of the 5.0 tests) could have a look at this PR, I would feel reassured.

Fixes: #10117
Closes: #10167

Ensures that trunk compiles again in -with-afl mode.

dra27

The original bug in #10107 was running tests/flambda/afl_lazy.ml in flambda mode on Windows, which is a mode not covered by CI testing at the moment, sadly. The dummy caml_setup_afl also needs to allocate the buffer and I think it's worth hardening caml_reset_afl_instrumentation, given that all this began with an "unexpected" configuration!

runtime/afl.c

dra27 · 2023-01-09T14:48:15Z

I've checked and tests/flambda/afl_lazy.ml passes locally on mingw-w64; @jmid are you able to test the actual afl side of things?

Octachron · 2023-01-09T14:58:08Z

Looking at the afl-fuzz-test test, the issue seems to be the afl directory and file layout rather than the test itself: the test looks for unique_crashes in <output-dir>/fuzzer_stats whereas the information on my system belongs to the saved_crashed field in <output-dir>/default/fuzzer_stats.

jmid · 2023-01-10T08:44:56Z

Thanks for the ping. I'm looking at it now. I'm getting a test failure locally which I'm investigating.

jmid

OK, I've now had a look at this.

On my Linux box by default systemd is set up to handle crashes - rather than do a good old core dump. AFL detects that and fails early, which caused the test to fail for me:
https://github.com/google/AFL/blob/61037103ae3722c8060ff7082994836a794f978e/afl-fuzz.c#L7326-L7347

I therefore needed to make the following change to get things running again:

index bb4329abd..29140fdb0 100755
--- a/testsuite/tests/afl-instrumentation/afl-fuzz-test.run
+++ b/testsuite/tests/afl-instrumentation/afl-fuzz-test.run
@@ -5,7 +5,7 @@ exec > ${output} 2>&1
 
 mkdir readline-input
 echo a > readline-input/testcase
-export AFL_SKIP_CPUFREQ=1 AFL_FAST_CAL=1 AFL_NO_UI=1 AFL_BENCH_UNTIL_CRASH=1
+export AFL_I_DONT_CARE_ABOUT_MISSING_CRASHES=1 AFL_SKIP_CPUFREQ=1 AFL_FAST_CAL=1 AFL_NO_UI=1 AFL_BENCH_UNTIL_CRASH=1
 timeout 10s afl-fuzz -m none -i readline-input -o readline-output ./readline > afl-output
 
 if [ `grep "unique_crashes" readline-output/fuzzer_stats | cut -d ':' -f 2` -gt 0 ];

The environment variable AFL_I_DONT_CARE_ABOUT_MISSING_CRASHES is not well documented, but I note that AFL has used it in its own CI tests.
With that in place the tests run smoothly for me.

Overall the code looks good to me. I'm wondering about potentially needless dynamic allocations in the non-AFL mode when there are multiple instrumented modules (commented inline above).

@Octachron Which version of afl are you using? I'm on 2.52b here (installed via opam) but I see unique_crashes is output in 2.57b too:
https://github.com/google/AFL/blob/master/afl-fuzz.c#L3470

jmid · 2023-01-10T12:56:57Z

runtime/afl.c

 {
+  /* AFL is not supported, but we still need to allocate space for the bitmap
+       (the instrumented OCaml code will write into it). */
+  caml_afl_area_ptr = caml_stat_alloc(INITIAL_AFL_AREA_SIZE);


Does this allocate for each of the instrumented modules, but only save the last one into the global pointer?
If so, we might consider guarding like the AFL case below in lines 90-91:
https://github.com/xavierleroy/ocaml/blob/84792cd3de00a07c0c555cfed45e2199f6d92cd9/runtime/afl.c#L90-L91

Indeed - the afl_initialised static from the other half also wants to be made common

You're right, the allocation and initialization should be done once per program, not once per instrumented module. I just pushed a fix, using caml_afl_area_ptr != NULL to detect previous initialization.

Octachron · 2023-01-10T13:59:27Z

Ah, I was using afl++ rather than afl which explains both the different format and why the showmap test didn't work due to difference in instrumentation. Sorry for the noise .

jmid · 2023-01-10T15:32:47Z

Ah, I was using afl++ rather than afl which explains both the different format and why the showmap test didn't work due to difference in instrumentation. Sorry for the noise .

To be fair, the original author is no longer maintaining afl and Google has archived the repo AFAIU. As such we should all move to a successor, e.g., afl++ eventually. Changing the dependency is better fit for another day and PR IMO.

Instrumented code produced by `ocamlopt -afl-instrument` but not run under AFL needs a 64 k dummy buffer at run-time. Before, this buffer was allocated statically, and therefore present in all ocamlopt-generated executables, whether compiled with `-afl-instrument` or without, whether AFL was selected at configuration-time or not. This commit implements dynamic allocation of the AFL buffer the first time an AFL-instrumented module runs its initialization code. It is based on the proposal at ocaml#10167 and its discussion. Fixes: ocaml#10117 Closes: ocaml#10167

xavierleroy · 2023-01-11T09:37:54Z

Re: testing, I was using afl++ indeed. That's what Ubuntu 22.04 suggested to get the afl-show command. And the afl package is now a transitional package that installs afl++.

jmid · 2023-01-11T15:31:47Z

LGTM

avsm · 2023-03-05T21:49:59Z

runtime/afl.c

+       (the instrumented OCaml code will write into it). */
+  if (caml_afl_area_ptr == NULL) {
+    caml_afl_area_ptr = caml_stat_alloc(INITIAL_AFL_AREA_SIZE);
+    memset(caml_afl_area_ptr, 0, INITIAL_AFL_AREA_SIZE);


Just an aside while looking over this PR: it might be useful to have a caml_stat_calloc call in the runtime that uses calloc() instead of malloc(); memset(). We only have caml_stat_calloc_noexc which wouldn't be much of a saving here, and it just uses memset() under the hood anyway.

Octachron

Green-checking on @jmid behalf.

Update, reorder and simplify inclusions of header files

e348b68

Ensures that trunk compiles again in -with-afl mode.

dra27 reviewed Jan 9, 2023

View reviewed changes

runtime/afl.c Show resolved Hide resolved

runtime/afl.c Outdated Show resolved Hide resolved

xavierleroy force-pushed the afl-dynamic-allocation branch from 839ef9f to 84792cd Compare January 9, 2023 14:29

jmid reviewed Jan 10, 2023

View reviewed changes

xavierleroy force-pushed the afl-dynamic-allocation branch from 84792cd to 55685d1 Compare January 11, 2023 09:33

jmid mentioned this pull request Jan 11, 2023

Document environment variables for running the testsuite #11886

Merged

Octachron self-assigned this Feb 6, 2023

avsm reviewed Mar 5, 2023

View reviewed changes

Octachron approved these changes Apr 4, 2023

View reviewed changes

Octachron merged commit cfeed85 into ocaml:trunk Apr 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AFL dynamic allocation #11877

AFL dynamic allocation #11877

xavierleroy commented Jan 9, 2023 •

edited

dra27 left a comment

dra27 commented Jan 9, 2023

Octachron commented Jan 9, 2023 •

edited

jmid commented Jan 10, 2023

jmid left a comment

jmid Jan 10, 2023

dra27 Jan 10, 2023

xavierleroy Jan 11, 2023

Octachron commented Jan 10, 2023

jmid commented Jan 10, 2023

xavierleroy commented Jan 11, 2023

jmid commented Jan 11, 2023

avsm Mar 5, 2023

Octachron left a comment

AFL dynamic allocation #11877

AFL dynamic allocation #11877

Conversation

xavierleroy commented Jan 9, 2023 • edited

dra27 left a comment

Choose a reason for hiding this comment

dra27 commented Jan 9, 2023

Octachron commented Jan 9, 2023 • edited

jmid commented Jan 10, 2023

jmid left a comment

Choose a reason for hiding this comment

jmid Jan 10, 2023

Choose a reason for hiding this comment

dra27 Jan 10, 2023

Choose a reason for hiding this comment

xavierleroy Jan 11, 2023

Choose a reason for hiding this comment

Octachron commented Jan 10, 2023

jmid commented Jan 10, 2023

xavierleroy commented Jan 11, 2023

jmid commented Jan 11, 2023

avsm Mar 5, 2023

Choose a reason for hiding this comment

Octachron left a comment

Choose a reason for hiding this comment

xavierleroy commented Jan 9, 2023 •

edited

Octachron commented Jan 9, 2023 •

edited