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

fuzz-introspector needs ldflags but projects do not always use #7540

Closed
DavidKorczynski opened this issue Apr 8, 2022 · 19 comments
Closed

Comments

@DavidKorczynski
Copy link
Collaborator

DavidKorczynski commented Apr 8, 2022

Fuzz Introspector sets LDFLAGS:

export LDFLAGS="-fuse-ld=gold"

however, far from all projects are using the LDFLAGS provided by OSS-Fuzz and it's causing many of those to fail building with fuzz introspector, (one example is: #7538 but there are more). I am trying to find out what's the preferred solution for passing LDFLAGS to builds.

There are traces in the code for LDFLAGS previously being set by OSS-Fuzz:

# TODO: remove after tpm2 catchup.
ENV FUZZER_LDFLAGS ""

but it seems like this was on the way out? As I see it there are some options

  1. Set LDFLAGS for fuzz-introspector as we currently do and an empty LDFLAGS for sanitizers (or use the FUZZER_LDFLAGS from above). This requires builds to use the LDFLAGS provided by OSS-Fuzz which may be a bit annoying -- I can do a run over the projects and get as many as possible. This would work but adds another requirement to builds.
  2. Try and do something by way of LIB_FUZZING_ENGINE, although I'm not sure I like this solution a whole lot.
  3. Keep it just as it is, but this will still force builds to use LDFLAGS (same as option (1) but will not set empty for non fuzz introspector builds) from OSS-Fuzz and perhaps some code as follows (e.g. icu: set LDFLAGS to fix fuzz-introspector #7538) :
# Fuzz introspector uses LDFLAGS, so ensure LDFLAGS
# is always set for other sanitizer options.
if [ "$SANITIZER" != "introspector" ]; then
  export LDFLAGS=""
fi
  1. something else?
@evverx
Copy link
Contributor

evverx commented Apr 11, 2022

far from all projects are using the LDFLAGS provided by OSS-Fuzz and it's causing many of those to fail building with fuzz introspector

I suspect that's because LDFLAGS has never been documented at https://google.github.io/oss-fuzz/getting-started/new-project-guide/#Requirements. Other than that OSS-Fuzz has always used CFLAGS to pass linker options around (which in turn led to issues like mesonbuild/meson#4542 and mesonbuild/meson#8345). It would be great if those LDFLAGS issues could be fixed generally once and for all. Until then I'm not sure passing LDFLAGS is a good idea considering that it works just because build systems like meson get around those issues.

@evverx
Copy link
Contributor

evverx commented Apr 11, 2022

Just to clarify what I was trying to say is that OSS-Fuzz should either keep passing linker flags via CFLAGS or start passing all the linker flags via LDFLAGS correctly. I don't think for example meson would (or should) support putting the linker flags in both CFLAGS and LDFLAGS and work that around every time it breaks.

@evverx
Copy link
Contributor

evverx commented Apr 11, 2022

Judging by 05be069 it was decided that LDFLAGS should be passed there. Since it effectively means that for example the systemd build system has to support four different modes of building the fuzz targets I wonder if it's possible to turn the fuzz-introspector off there? I'm not sure it's maintainable at this point.

@DavidKorczynski
Copy link
Collaborator Author

DavidKorczynski commented Apr 11, 2022

Thanks @evverx -- 05be069 may be temporary depending on what we decide here.

I think you're view on using CFLAGS is valid, however, I think it may have some complications with projects that do not pass on CFLAGS onto linking, e.g. which is likely the case for many projects that link without using CC. I ran into several builds that will fail if we don't have it in LDFLAGS, and often the failure happens when shared libraries are linked before reaching the fuzzing compilation part of the build process. I'll investigate more in-depth the consequences of doing it by way of CFLAGS in the near future though.

Regardless of what we decide here then I think projects should be able to opt out of fuzz-introspector.

@DavidKorczynski
Copy link
Collaborator Author

I just checked the fuzz-introspector build status for systemd @evverx and it looks to be passing atm? You can see the build status for fuzz-introspector on oss-fuzz.com and build status

@evverx
Copy link
Contributor

evverx commented Apr 11, 2022

I think it may have some complications with projects that do not pass on CFLAGS onto linking, e.g. which is likely the case for many projects that link without using CC

I think most projects should be compatible with CFLAGS and CC. Otherwise they would fail to compile with afl++ and/or coverage. My understanding is that autoconf/automake has always supported putting everything in CFLAGS/CFLAGS and other build systems (including meson) had to follow suit. This compatibility is the reason why issues like mesonbuild/meson#4542 are fixed when they pop up. Mixing linker options in both CFLAGS and LDFLAGS is kind of a gray area and as far as I know it's unlikely that meson will ever support that.

I just checked the fuzz-introspector build status for systemd @evverx and it looks to be passing atm?

It does but it isn't officially supported by meson and a new version of meson can break that.

@DavidKorczynski
Copy link
Collaborator Author

DavidKorczynski commented Apr 11, 2022

My understanding of why afl++ works is because afl++ deploys its own linker (it is an issue on fuzz-introspector to use a similar approach ossf/fuzz-introspector#57 , but am not sure if it will come with unintended consequences though). Am not entirely sure about the details of the coverage though.

Interesting with autocon/automake -- I think if we can push it into CFLAGS that would be the best -- again, will take a look at this in more detail in the near future.

@evverx
Copy link
Contributor

evverx commented Apr 11, 2022

My understanding of why afl++ works is because afl++ deploys its own linker

I think this is kind of hidden behind CC and CXX so if projects use CC, CXX, CFLAGS and CXXFLAGS as documented in https://google.github.io/oss-fuzz/getting-started/new-project-guide/#Requirements they should automagically be buildable with AFL. There have been issues like #5691 though where it wasn't clear what failed and why but as long as it's possible to temporarily turn it off to let OSS-Fuzz keep fuzzing projects it's safe to say that it's just a minor inconvenience.

DavidKorczynski added a commit to DavidKorczynski/oss-fuzz that referenced this issue Apr 15, 2022
The use of LDFLAGS does not really follow the policy of OSS-Fuzz. This
moves the linker flags into the sanitizer flags.

Ref:
google#7540 (comment)
DavidKorczynski added a commit that referenced this issue Apr 15, 2022
* fuzz-introspector: remove use of LDFLAGS

The use of LDFLAGS does not really follow the policy of OSS-Fuzz. This
moves the linker flags into the sanitizer flags.

Ref:
#7540 (comment)
@DavidKorczynski
Copy link
Collaborator Author

DavidKorczynski commented Apr 18, 2022

The set up was switched to pass the linker flags by way of cflags in #7573

I went through the OSS-Fuzz logs to identify those that went from success builds to failed builds (16th April success, 17th April fail), and got the following list:

arrow
botan
cel-cpp
croaring
duckdb
flatbuffers
fluent-bit
freeimage
freeradius
ghostscript
grpc-httpjson-transcoding
hiredis
hoextdown
http-pattern-matcher
icu
librawspeed
librdkafka
libzmq
opencensus-cpp
openh264
pcapplusplus
perfetto
tcmalloc
unrar
upd
xvid

I skimmed through some of the logs and it looks like some parts of the builds do not use lto for the compiling/linking which causes some undefined references. As far as I could tell this often happens because of some third party dependencies that are compiled from source. However, will have to look at them in more detail.

@evverx
Copy link
Contributor

evverx commented Apr 19, 2022

@DavidKorczynski having taken a look at #7573 a bit closer I think -fuse-ld=gold should be passed without -Wl:

clang -fuse-ld=gold -v -x c <(echo 'int main () { return 0; }')
...
"/usr/bin/ld.gold" --hash-style=gnu ...

With -Wl and LDFLAGS=-fuse-ld=gold it seems to be broken:

clang -Wl,-fuse-ld=gold -v -x c <(echo 'int main () { return 0; }')
...
"/usr/bin/ld" --hash-style=gnu ...
LDFLAGS=-fuse-ld=gold clang -v -x c <(echo 'int main () { return 0; }')
...
"/usr/bin/ld" --hash-style=gnu ...

@evverx
Copy link
Contributor

evverx commented Apr 19, 2022

In theory it should fix the projects where lld fails when -fuse-ld=gold is passed to it

@evverx
Copy link
Contributor

evverx commented Apr 19, 2022

-flto should be passed without -Wl as well. It should fix the projects where gold complains that "/usr/bin/ld.gold: fatal error: -f/--auxiliary may not be used without -shared".

@evverx
Copy link
Contributor

evverx commented Apr 19, 2022

Looking at the logs I have to admit I'm puzzled as to how it worked with LDFLAGS. Some projects appear to default to lld so they are always linked with it regardless of what LDFLAGS contains. Projects like apel-cpp seem to default to gold but they seem to ignore -flto passed via LDFLAGS (otherwise they would have failed with /usr/bin/ld.gold: fatal error: -f/--auxiliary may not be used without -shared). My guess would be that they are built with lto by default maybe. I have to admit I'm not sure how to make it all work.

DavidKorczynski added a commit to DavidKorczynski/oss-fuzz that referenced this issue Apr 19, 2022
Moves -fuse-ld=gold to compile flags and removes -flto from linker
flags.

Should fix a number of the projects
google#7540 (comment)

Ref:
google#7540 (comment)

Ref:
google#7540 (comment)
@DavidKorczynski
Copy link
Collaborator Author

DavidKorczynski commented Apr 19, 2022

Thanks a ton for the help here @evverx. I went through several of the projects and confirmed your suggestions made the projects build. This #7583 should fix it up in the infrastructure. Hopefully that should make some optimal number of the projects to build.

@evverx
Copy link
Contributor

evverx commented Apr 19, 2022

@DavidKorczynski FWIW I think if that PR fixes most of the projects it should be good to go (as long as fuzz-introspector build failures aren't reported on monorail and the badge doesn't turn yellow/red).

@DavidKorczynski
Copy link
Collaborator Author

DavidKorczynski commented Apr 19, 2022

@DavidKorczynski FWIW I think if that PR fixes most of the projects it should be good to go (as long as fuzz-introspector build failures aren't reported on monorail and the badge doesn't turn yellow/red).

Yeah I think so too - I wanted to give the devs from the meson team a bit of time to reply to your message before merging, however, we can always carry on the conversation after so will make it ready for review tomorrow -- it would be nice to merge to see the impact on all the projects

@evverx
Copy link
Contributor

evverx commented Apr 20, 2022

I think currently it's possible to for example set the linker to gold with CC_LD=gold/CXX_LD=gold, LDFLAGS=-fuse=gold and it seems neither of those options is satisfactory. To handle something like CFLAGS=-fuse-ld=gold meson would have to treat it differently and I'm not sure the meson project would be open to that (CFLAGS=-Wl,-fuse-ld=gold somewhat works in the sense that it doesn't break compiler checks but gold isn't actually used anywhere).

@evverx
Copy link
Contributor

evverx commented Apr 20, 2022

I think another option would be to bring LDFLAGS back but all the projects where LDFLAGS isn't used would have to be patched by analogy with #7538 (which isn't perfect either).

@DavidKorczynski
Copy link
Collaborator Author

Yeah, I think at the moment I prefer the approach in #7583 because it follows the oss-fuzz way of doing things more, and using the previous approach will require modifying the remaining projects to be like #7538 may be undesired. #7583 is not ideal though and will probably require some hacky things in other projects -- I say let's merge #7583 in and see how the list of projects react in terms of how many build/fail

DavidKorczynski added a commit that referenced this issue Apr 20, 2022
Moves -fuse-ld=gold to compile flags and removes -flto from linker
flags.

Should fix a number of the projects
#7540 (comment)

Ref:
#7540 (comment)

Ref:
#7540 (comment)
MartinPetkov pushed a commit to MartinPetkov/oss-fuzz that referenced this issue Aug 15, 2022
* fuzz-introspector: remove use of LDFLAGS

The use of LDFLAGS does not really follow the policy of OSS-Fuzz. This
moves the linker flags into the sanitizer flags.

Ref:
google#7540 (comment)
MartinPetkov pushed a commit to MartinPetkov/oss-fuzz that referenced this issue Aug 15, 2022
Moves -fuse-ld=gold to compile flags and removes -flto from linker
flags.

Should fix a number of the projects
google#7540 (comment)

Ref:
google#7540 (comment)

Ref:
google#7540 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants