Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AFL stability fixes for objects (MPR#7725) and lazy values. #1754

Merged
merged 5 commits into from May 4, 2018

Conversation

Projects
None yet
2 participants
@stedolan
Copy link
Contributor

commented May 2, 2018

For AFL fuzzing to be useful, code that does the same thing twice should produce exactly the same instrumentation output both times, but this currently fails for objects (see MPR#7725) and lazy values.

For objects, the class initialisation logic in camlinternalOO.ml uses some global state as a cache. Since this is not visible outside this file, the fix is just to disable instrumentation for this file (even if instrumentation is otherwise globally enabled).

For lazy values, the GC can silently replace Forward_tag values (forced lazy thunks) with their contents, causing the inlined Lazy.force test to branch differently. The fix is to stop inlining this test if AFL instrumentation is enabled, and ensure that camlinternalLazy.ml is never instrumented.

stedolan added some commits Dec 11, 2017

Ensure Lazy has stable behaviour with afl-instrument.
When AFL instrumentation is enabled, the inlining of Lazy.force
is disabled, so that the GC optimisation of removing Forward_tag
blocks is no longer visible in the instrumentation output.

@stedolan stedolan force-pushed the stedolan:afl-stability-fixes branch from 63296af to 07e20ab May 2, 2018

@gasche

gasche approved these changes May 3, 2018

Copy link
Member

left a comment

I believe that the change is correct and an improvement for afl-fuzz user. Just like #1345, it also results in a performance degradation when using afl instrumentation -- an acceptable price for more robust bug-finding capabilities.

For the record (for other people than Stephen), I looked a bit at whether a more general solution to these stability problems would be possible, in stedolan/ocaml-afl-persistent#2; this was a prototype and my take-away was that a substantial rework of the afl-instrumentation layer would be required to do substantially better than the present work-arounds. (So: interesting, but requires more time than we have right now.)

if !Clflags.afl_instrument then
(* Disable inlining optimisation if AFL instrumentation active,
so that the GC forwarding optimisation is not visible in the
instrumentation output. (PR#???) *)

This comment has been minimized.

Copy link
@gasche

gasche May 3, 2018

Member

I think that stedolan/crowbar#14 would be a good pointer to give here, in addition to GPR#1754 itself.

This comment has been minimized.

Copy link
@stedolan

stedolan May 3, 2018

Author Contributor

Thanks! I was looking for that issue, but I thought it was on Mantis.

@gasche

This comment has been minimized.

Copy link
Member

commented May 3, 2018

@stedolan doesn't this PR suggest an alternative fix for #1345 that would degrade performances less? Instead of completely disabling the cache lookup (that is currently inlined within generated code), maybe you could define its logic as a function in CamlinternalOO?

@gasche

This comment has been minimized.

Copy link
Member

commented May 3, 2018

(I'm planning to eventually merge this (in trunk only), but would like to leave a few days for people to chime in if they wish to.)

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2018

I agree that the approach in stedolan/ocaml-afl-persistent#2 is promising-but-work.

Using this approach as an alternative to #1345 is challenging. Since class initialisation imports an arbitrary amount of stuff from the current environment, it can't easily be pulled out to a library function (at least, not without allocating a new closure, which would defeat the point of the optimisation). For instance, compiling fun a b c -> object method a = a method b = b method c = c end results in the following lambda:

(let
  (shared/1326 =a [0: #"a" #"b" #"c"]
   shared/1319 =a [0: #"c" #"b" #"a"]
   class_tables/1307 =a (makemutable 0 0a 0a 0a))
  (function a/1290 b/1291 c/1292
    (funct-body //toplevel//(1):0-62
      (before //toplevel//(1):13-62
        (let (cached/1325 =a class_tables/1307)
          (seq
            (if (field 0 cached/1325) 0a
              (let
                (class/1311 =
                   (apply (field 15 (global CamlinternalOO!)) shared/1326)
                 env_init/1322 =
                   (... complicated code mentioning a, b, c ...))
                (seq (apply (field 16 (global CamlinternalOO!)) class/1311)
                  (setfield_ptr 0 cached/1325 env_init/1322))))
            (let (envs/1324 =a (makeblock 0 c/1292 b/1291 a/1290))
              (apply (field 0 cached/1325) envs/1324))))))))

@gasche gasche merged commit cfee1d8 into ocaml:trunk May 4, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vicuna vicuna referenced this pull request Mar 14, 2019

Closed

AFL test broken #7725

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.