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

aflplusplus: 2.59c -> 2.64c #85950

Merged
merged 2 commits into from May 6, 2020
Merged

aflplusplus: 2.59c -> 2.64c #85950

merged 2 commits into from May 6, 2020

Conversation

@Mindavi
Copy link
Contributor

Mindavi commented Apr 24, 2020

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip" -> There are no dependencies
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc: @risicle

@risicle
Copy link
Contributor

risicle commented Apr 24, 2020

Neat, a toy to play with over the weekend...

@risicle
Copy link
Contributor

risicle commented Apr 25, 2020

Oh we should probably change the fetchFromGitHub owner to AFLplusplus now they've got an organization, and they also seem to have an actual homepage @ https://aflplus.plus/

@risicle
Copy link
Contributor

risicle commented Apr 25, 2020

I've got qemu building in 229f58b

Now I'm getting a failure building unsigaction32...

@risicle
Copy link
Contributor

risicle commented Apr 25, 2020

Oh but it seems unsigaction32's build is broken anyway AFLplusplus/AFLplusplus@079fdbf#commitcomment-38428357 so we'll just have to skip it.

(see d4e6258)

@risicle
Copy link
Contributor

risicle commented Apr 25, 2020

Ok and I've got the tests working again in 5fee991

I also added cmocka to get the "unit tests" working, and they do work - not sure of the value though...?

I also haven't used it yet.

@Mindavi
Copy link
Contributor Author

Mindavi commented Apr 26, 2020

Some discussion points:

  1. Not sure how to commit this while acknowledging your changes. Is mentioning you in the commit ok or how is this typically done? If I squash I'm the only author, afaik.
  2. Should the clang_9 and llvm_9 variables be defined in the top-level/all-packages.nix file or is it ok as-is? The advantage of doing it in the top-level could be that someone can override them once clang-10 and llvm-10 are out of RC phase, I think. At least, that's what I think the top-level file does.
@risicle
Copy link
Contributor

risicle commented Apr 26, 2020

  1. Don't worry about acknowledging my commits, squash them, actually probably worth a mention to spread the blame when it all blows up ;)
  2. If there is a canonical answer to this, I don't know it. I guess it depends to what extent we find it works with other llvm versions (I haven't tried others, I have to compile llvm enough as it is...). e.g. if we find it blows up with anything other than llvm_9, there's little point in giving users that knob to twiddle.
@risicle
Copy link
Contributor

risicle commented Apr 26, 2020

Hmm, I can't get qemu_mode/libcompcov qemu_mode/unsigaction to build on i686. Which is annoying because the wine mode only works in 32bit qemu mode.

@Mindavi
Copy link
Contributor Author

Mindavi commented Apr 26, 2020

Seems to be possible to use an older llvm/clang version, but I don't think that's too useful for most people. Let's just leave them out of the configurable options.

@Mindavi
Copy link
Contributor Author

Mindavi commented Apr 26, 2020

Maybe you should create an issue upstream, if you think the issue lies there.

@Mindavi Mindavi force-pushed the Mindavi:aflplusplus/2.64c branch from ceaf01f to cacce4e Apr 26, 2020
@risicle
Copy link
Contributor

risicle commented Apr 26, 2020

I've managed to get unsigaction building acceptably again by applying the fix that's already in master:

    patches = [
      (fetchpatch {
        url = "https://github.com/AFLplusplus/AFLplusplus/commit/5b9928f1a9d4b017ea04365ca8b522fde71236eb.patch";
        sha256 = "1m4w9w4jaxb2mjkwvr6r4qa2j5cdzzpchjphpwd95861h0zvb6hh";
      })
    ];

This also fixes build on i686 for me.

@Mindavi Mindavi force-pushed the Mindavi:aflplusplus/2.64c branch from cacce4e to 7a3392b Apr 28, 2020
@Mindavi
Copy link
Contributor Author

Mindavi commented Apr 28, 2020

I've applied the patch you sent in. It seems to be working ok, but I'm on a 64-bit system. Should the unsigaction32.so be built on a 64-bit system? It doesn't build for me. Only unsigaction64.so seems to build for me.

@risicle
Copy link
Contributor

risicle commented Apr 28, 2020

I find that unsigaction32 only builds on 32bit, unsigaction64 only on 64bit, and you can test i686 easily because it's all available under pkgsi686Linux.aflplusplus.

However, I can't get even a straightforward afl-clang-fast build based on this package to work for me:

[-] PROGRAM ABORT : Oops, failed to execute 'clang' - check your PATH
             Location : main(), afl-clang-fast.c:846

This is when I afl-build yajl by adding

  AFL_HARDEN="1";
  AFL_LLVM_LAF_SPLIT_SWITCHES="1";
  AFL_LLVM_LAF_TRANSFORM_COMPARES="1";
  AFL_LLVM_LAF_SPLIT_COMPARES="1";
  preConfigure = ''
    export CC=${aflplusplus}/bin/afl-clang-fast
  '';

to its attrs.

@risicle
Copy link
Contributor

risicle commented Apr 28, 2020

I'm pretty sure were still going to need to bind the afl tools to their underlying compilers, if you run strings on the resulting afl-clang-fast binary you can see it has references to the llvm-9.0.0 package, but that package doesn't actually have clang only llvm utilities.

@Mindavi
Copy link
Contributor Author

Mindavi commented Apr 28, 2020

The way I do this is by making a shell like so:

with import /home/rick/nixpkgs-aflplusplus {}; {
  qpidEnv = stdenvNoCC.mkDerivation {
    name = "fuzz-env";
    buildInputs = [
      pkgs.clang_9
      pkgs.llvm_9
      pkgs.cmake
      pkgs.aflplusplus
      pkgs.pkg-config
      pkgs.gcc9
    ];
  };
}

Note the pkgs.clang_9. I've looked at the documentation about this and there are ways to pass these dependencies through, but I wasn't sure if we should do so. It looks like depsTargetTarget might be what we need, but there is a disclaimer:

A list of dependencies whose host platform matches the new derivation's target platform. This means a 1 offset from the new derivation's platforms. These are packages that run on the target platform, e.g. the standard library or run-time deps of standard library that a compiler insists on knowing about. It's poor form in almost all cases for a package to depend on another from a future stage [future stage corresponding to positive offset]. Do not use this attribute unless you are packaging a compiler and are sure it is needed.

s.a.: https://nixos.org/nixpkgs/manual/#ssec-stdenv-dependencies

Maybe just putting clang_9 in the buildInputs would be fine too, not sure about that.

Edit: yeah, I think it should be in buildInputs. Will try shortly.

@risicle
Copy link
Contributor

risicle commented Apr 28, 2020

Even if you are able to pass these extras through, these will be runtime-resolved dependencies (if you coincidentally end up with the another clang in your runtime env it will break), and a well built package shouldn't have to rely on its runtime environment to work properly.

We also shouldn't unnecessarily spew our dependencies into the package user's runtime env, because it makes using more than one package which has taken this attitude at the same time almost impossible.

@Mindavi
Copy link
Contributor Author

Mindavi commented Apr 28, 2020

Ah yes, thanks for the insights!

You're completely right, I didn't think through that the implicit dependency (without having the correct env variables set) during runtime makes the package impure.

However, I do think that there's a runtime dependency on clang (and AFL assumes the version that it's built with, too, as far as I can see. Some optimizations are enabled/disabled based on the version of the compiler used). So I'm not sure if we shouldn't pass it through, because otherwise you'll always have to specify the correct version of clang, I think. That doesn't make the package more friendly to use and I'm not sure why it'd be beneficial if you could use afl without having clang in your environment.

@risicle
Copy link
Contributor

risicle commented Apr 28, 2020

You shouldn't have to - unless for some reason you need to also call the unwrapped clang during a build process. Otherwise the correctly wrapped afl-clang-fast will do everything you need clang to do, and its' wrapper's reference to our special clang will get picked up by nix as a runtime dependency - it just won't get injected into the target's PATH etc. (which TBH I think would just be confusing and a bit of a footgun)

It's certainly worked for me in quite heavy use for most of a year now.

It's helped by the fact that clang is near 100% interchangeable with gcc and its tools on linux (doesn't need its own linker etc., GNU tools are quite happy with its output)

@Mindavi
Copy link
Contributor Author

Mindavi commented May 1, 2020

Applying the following patch adds the full path to the clang binary inside the built binary. However, AFL_PATH should still be set as well (to prevent any issues with loading the shared libs). Maybe the hack you used is the best method after all, not sure.

+
+    postPatch = ''
+      # Replace the CLANG_BIN variables with the correct path
+      substituteInPlace llvm_mode/afl-clang-fast.c \
+        --replace "CLANGPP_BIN" '"${clang_9}/bin/clang++"' \
+        --replace "CLANG_BIN" '"${clang_9}/bin/clang"'
+    '';
+

I've also found out that AFL (depending on what you build) links afl-clang either to afl-clang-fast or to afl-gcc. I'm not sure what's best, but we could consider just removing it. It doesn't add anything if it's symlinked to afl-clang-fast anyway.

@risicle
Copy link
Contributor

risicle commented May 2, 2020

Ah I hadn't considered patching the source - there's definitely a neatness to it. It should be possible to patch AFL_PATH in as a "default" too a la

--replace 'getenv("AFL_PATH")' "(getenv(\"AFL_PATH\") ? getenv(\"AFL_PATH\") : \"$out/lib/afl\")"

the only disadvantage of this I see is it being more sensitive to source changes, which we're kind of invulnerable to using wrappers. But then again, using wrappers adds a few runtime weirdness possibilities...

🤷

@risicle
Copy link
Contributor

risicle commented May 2, 2020

As for afl-clang I think they discontinued it because afl-clang-fast was better in every way and now they just symlink to afl-clang-fast it for compatibility.

@Mindavi Mindavi force-pushed the Mindavi:aflplusplus/2.64c branch from 7a3392b to 9805c98 May 2, 2020
@Mindavi
Copy link
Contributor Author

Mindavi commented May 2, 2020

I've pushed new changes with the source changes. I've removed the symlinks from afl-clang to afl-clang-fast. I've applied the getenv("AFL_PATH") patch as well.

@risicle
Copy link
Contributor

risicle commented May 2, 2020

Neat - do we need to do similar for afl-gcc / afl-gcc-fast?

@Mindavi
Copy link
Contributor Author

Mindavi commented May 4, 2020

Yes, we'd need to do the same.

One advantage of doing it this way is that you can still pass the AFL_CC and AFL_CXX to the compilers.

Will look into doing the same for gcc now.

Do we want to support gcj as well? It's a java compiler that's part of the gcc collection, but it has been deprecated according to wikipedia, so I don't think we should support it. I'll ask upstream if they'd like to keep it or what they think.

It was part of the GNU Compiler Collection for over ten years but as of 2017 it is no longer maintained and will not be part of future releases.

Edit: started discussion about this upstream as well AFLplusplus/AFLplusplus#184 (comment)

Edit2: A patch has been applied for afl-gcc. afl-gcc-fast already picks up the correct gcc from the PATH during build time (it's passed as a define), and inspection reveals the path is indeed hardcoded.

I think this makes the whole thing ready for review. I can't seem to test the 32-bit binaries, as I get failures that show that clang can't be built for 32-bit systems. Could you check this @risicle? We might need to rebase master against this if it keeps failing. I've tried building it with nix-shell -I nixpkgs=~/nixpkgs-aflplusplus/ -p pkgsi686Linux.aflplusplus.

@Mindavi Mindavi force-pushed the Mindavi:aflplusplus/2.64c branch 3 times, most recently from 6d08bec to 23d650f May 4, 2020
@Mindavi Mindavi marked this pull request as ready for review May 4, 2020
Mindavi added 2 commits Apr 24, 2020
Updated together with maintainers.ris / @risicle.
@Mindavi Mindavi force-pushed the Mindavi:aflplusplus/2.64c branch from 23d650f to 416c891 May 4, 2020
@risicle
Copy link
Contributor

risicle commented May 5, 2020

This seems to be working... think this is ready?

@risicle
Copy link
Contributor

risicle commented May 6, 2020

One annoying bug 2.64 has is AFLplusplus/AFLplusplus#342 unfortunately the patch won't apply cleanly. Will have to wait for 2.65.

@Mindavi
Copy link
Contributor Author

Mindavi commented May 6, 2020

Let's do that then. It's quite a fast-moving target so it's hard to keep track of all changes and ensure everything works as intended.

@risicle risicle merged commit e153004 into NixOS:master May 6, 2020
16 checks passed
16 checks passed
aflplusplus, aflplusplus.passthru.tests on aarch64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
aflplusplus, aflplusplus.passthru.tests on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="416c891"; rev="416c8914618c315863131903e6ced09f74165ad3"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="416c891"; rev="416c8914618c315863131903e6ced09f74165ad3"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="416c891"; rev="416c8914618c315863131903e6ced09f74165ad3"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="416c891"; rev="416c8914618c315863131903e6ced09f74165ad3"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="416c891"; rev="416c8914618c315863131903e6ced09f74165ad3"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="416c891"; rev="416c8914618c315863131903e6ced09f74165ad3"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="416c891"; rev="416c8914618c315863131903e6ced09f74165ad3"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@risicle
Copy link
Contributor

risicle commented May 6, 2020

:high-five:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.