Issue #110 - Fix afl-clang-fast -E and -shared regressions.

googlebot commented 4 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla[bot] commented 4 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

choller commented 4 years ago

CLA is signed by Mozilla.

sylvestre commented 4 years ago

@Dor1s Hello I haven't heard of a Mozilla/Google github group for this. However, we have corporate agreement between our companies.

Dor1s commented 4 years ago

@sylvestre Hello! Yeah, I'm able to see Mozilla among the corporate signers, and in the worst case can just ignore the CLA check here.

However, there is definitely a way to make the check pass. It might be nice to achieve that to avoid any confusion with future: https://cla.developers.google.com/about#add-contributors

As I understand, there must be some Google Group that was submitted with the corporate CLA.

sylvestre commented 4 years ago

Sure, I will see on our side what we can do to do the right thing :) thanks for the quick answer and pointer!

Dor1s commented 4 years ago

thank you @sylvestre !

and in the meantime we're waiting for @@andreafioraldi to take a look

andreafioraldi commented 4 years ago

LGTM, I will test it tomorrow morning (I'm in EU) but it should be ok, it doesn't revert my patch. Btw my use case is openssl on Fuzzbench, so if you want to test it now in order to merge ASAP just try to compile it using afl-clang-fast.

andreafioraldi commented 4 years ago

-shared reintroduces the https://github.com/google/fuzzbench/issues/110 bug. IMO -E is really needed, and fix the @choller compilation failures, -shared instead breaks the linking with errors about undefined references to afl-llvm-rt.

It is true that if we compile afl-llvm-rt into all the shared objects they will have duplicated code, however that code will be not used. In classic mode, afl_area_ptr is loaded from the .got section and so the loader will resolve the reference to a single shared object. Same for pcguard mode, sanitizer_cov_trace_pc_guard and __sanitizer_cov_trace_pc_guard_init are compiled into each shared objects, but only one from the first shared object is resolved by the loader so there are no conflicts.

We should keep the -E case and drop -shared.

choller commented 4 years ago

-shared reintroduces the google/fuzzbench#110 bug. IMO -E is really needed, and fix the @choller compilation failures, -shared instead breaks the linking with errors about undefined references to afl-llvm-rt.

This indicates that the project is linking an executable without attaching the runtime, and I would consider that a bug. It would mean that the runtime is only present because a shared object with the runtime is loaded into the executable and that needs fixing.

It is true that if we compile afl-llvm-rt into all the shared objects they will have duplicated code, however that code will be not used. In classic mode, __afl_area_ptr is loaded from the .got section and so the loader will resolve the reference to a single shared object.

I'm not talking about multiple shared objects, but shared object vs. the binary loading it. If both have the runtime, then I think there is no way for the loader to resolve this properly.

I also just looked up why this was added initially:

+--------------
+Version 2.06b:
+--------------
+
+  - Worked around LLVM persistent mode hiccups with -shared code.
+    Contributed by Christian Holler.

So this was causing real malfunctions with persistent mode.

I also think it's not useful to argue that this breaks one project in fuzzbench, because the change you introduced broke another project even before. I think we should stick to the way sanitizers solve this, because they face similar problems with runtime duplication (attaching for example the ASan runtime to shared objects will crash your process).

choller commented 4 years ago

I think an intermediate solution would be to add an environment variable that allows overriding this behavior. Something like AFL_ATTACH_RT_SHARED to force attaching the runtime even to shared objects. It should be clearly documented though that this can have severe side effects and might indicate a problem in the build system.

andreafioraldi commented 4 years ago

+1 for AFL_ATTACH_RT_SHARED

choller commented 4 years ago

+1 for AFL_ATTACH_RT_SHARED

Great, I'll change the PR accordingly. We should still investigate long-term why the OpenSSL build fails in this way, because that might be a bug.

choller commented 4 years ago

@andreafioraldi suggested to use -Wl,--dynamic-list to make sure the AFL symbols are always exported from the main binary. According to both of our tests, this seems to solve the problems with shared libraries (which only occurs btw when the library is loaded with dlopen()).

andreafioraldi commented 4 years ago

I forgot to put sancov in the list when I wrote it on Discord, push this updated list:

{
  "__afl_area_ptr";
  "__afl_manual_init";
  "__afl_persistent_loop";
  "__afl_auto_init";
  "__afl_area_initial";
  "__afl_prev_loc";
  "__sanitizer_cov_trace_pc_guard";
  "__sanitizer_cov_trace_pc_guard_init";
};

lszekeres commented 4 years ago

Thanks @choller and @andreafioraldi for updating the PR! LGTM.

@sylvestre, let us know if there's any update re the CLA, thanks!

choller commented 2 years ago

The CLA check here should be fixed.

Dor1s commented 2 years ago

@googlebot ping

google / AFL