firefox #1546

pdknsk · 2018-06-21T00:59:11Z

This is a preliminary PR for comments and discussion. It's complete, except for a bit of error handling. Read #1462 for previous discussion. The targets work locally with run_fuzzer.

I initially tried to tackle the problem with the many required libraries by using patchelf. Worked for some at first, or so it seemed, but those dependencies came back through other libraries. Since many libraries were going to be required anyway, I went for this single approach, rather than perhaps patching a few libraries out in addition to that.

It's lacking corpus and dictionary handling, which is trivial to add. I didn't make any, because I'm not familiar with the formats. Those would be hosted in mozilla-central anyway. If they already are, I couldn't find them.

alex

Rough first-cut review.

Thanks for working on this! This is pretty exciting.

alex · 2018-06-21T14:48:48Z

projects/firefox/build.sh

+# Build a wrapper binary for each target to set environment variables.
+for FUZZ_TARGET in ${FUZZ_TARGETS[@]}
+do
+cat << EOF > $FUZZ_TARGET.c


Put this is a normal C file and use defines to include the various compile-time parameters

alex · 2018-06-21T14:49:38Z

projects/firefox/build.sh

+  // Temporary (or permanent?) work-around for a bug in the fuzzing interface.
+  // https://bugzilla.mozilla.org/show_bug.cgi?id=1466021#c9
+  char* options = getenv("ASAN_OPTIONS");
+  strcat(options, ":detect_stack_use_after_return=0");


I don't think this is safe at all

You're right. I'm surprised now that it even works. Probably overwrites an unimportant variable name.

alex · 2018-06-21T14:49:53Z

projects/firefox/project.yaml

+homepage: "https://www.mozilla.org/firefox/"
+primary_contact: "agaynor@mozilla.com"
+fuzzing_engines:
+- none


I don't think this is right, probably should be libFuzzer?

alex · 2018-06-21T14:50:23Z

projects/firefox/project.yaml

+fuzzing_engines:
+- none
+sanitizers:
+- address


At a minimum we also want @choller in the CCs, probably some other folks as well

pdknsk · 2018-06-25T20:37:18Z

I'll address the points in an updated PR, but I'd also like an opinion of a Googler on the general approach.

pdknsk · 2018-07-05T17:21:49Z

Please take a look. I noticed from the build logs that it builds on 32 vCPU instances, so I updated the timings for that. With code downloading, it produces target binaries in 30-35 minutes.

oliverchang

This seems OK, with the caveat that since this is kind of unsupported it may break at any time on our production infrastructure. Are there any long term plans to better break up firefox's targets?

Also, please test this with the run_minijail script in the base-runner image like so:

$ run_minijail /out/target_name

To make sure things still work under minijail.

oliverchang · 2018-07-09T07:01:44Z

projects/firefox/target.c

+    perror("LD_LIBRARY_PATH unexpectedly set");
+    exit(1);
+  }
+  if (setenv("LD_LIBRARY_PATH", STRINGIFY(LIB_PATH), 0)) {


Can we make this dynamic based on the cwd and argv[0]? the binaries are not guaranteed to be executed from the same $OUT. Alternatively, patch rpaths using $ORIGIN.

oliverchang · 2018-07-09T07:03:18Z

projects/firefox/target.c

+  }
+  free(new_options);
+
+  return execv(STRINGIFY(FIREFOX_BINARY), argv);


same here, the binaries won't always be in $OUT.

oliverchang · 2018-07-09T07:04:15Z

projects/firefox/build.sh

+# These are taken from running ldd on libraries build for Firefox, with the
+# output manually filtered for libraries which are available on base-runner.
+# Patching even some libraries out doesn't work (undefined symbol errors).
+REQUIRED_LIBRARIES=(


can these be statically linked in somehow?

No, we discussed this before. It is not easily possible to link some system level dependencies statically.

choller · 2018-07-09T10:26:35Z

Are there any long term plans to better break up firefox's targets?

No. Breaking up the Firefox build process would require massive architectural changes in the project itself and that is currently not on the Firefox engineering agenda and probably won't be any time soon.

pdknsk · 2018-07-13T03:19:51Z

I hadn't tried run_minijail because I had assumed run_fuzzer calls it internally. Works now.

oliverchang

I think we can give this a shot now, if run_minijail works.

Are there any more comments from firefox folks? Once again this is a rather unsupported way of doing things and may break frequently.

In the long run to better support this, I think we need some way to let projects specify their own dependencies in the runner, and unify the OSS-Fuzz runner image with our actual runner images used on ClusterFuzz.

pdknsk · 2018-07-20T02:38:12Z

Once again this is a rather unsupported way of doing things and may break frequently.

Where do you identify the likely breaking points? If it's the many required shared libraries potentially changing frequently, then the good news is that this can actually be automated quite nicely.

alex

One small comment, otherwise LGTM. Will be interesting to see the results!

alex · 2018-07-25T14:23:00Z

projects/firefox/build.sh

+
+# Case-sensitive names of internal Firefox fuzzing targets. Edit to add more.
+FUZZ_TARGETS=(
+  ContentParentIPC


Let's remove this one for now -- as we discussed on the Firefox bug tracker, running the IPC fuzzer requires a few other env vars, otherwise we get a bunch of uninteresting crashes. We should add this back soon, but let's get the initial PR landed and then circle back.

pdknsk · 2018-07-26T06:10:40Z

It seems (from the public build logs) the first build failed with a network error. Bad luck.

* firefox * make wrapper its own file * many minor changes * build package rather then copying * add (disabled) Qcms target * getenv doesn't use errno * make paths dynamically * minor find fix * handle absolute path * make run_minijail work * get required libraries automatically * Remove ContentParentIPC fuzz target

firefox

27b623c

pdknsk mentioned this pull request Jun 21, 2018

firefox micro-targets #1462

Closed

alex reviewed Jun 21, 2018

View reviewed changes

pdknsk added 4 commits July 5, 2018 18:37

make wrapper its own file

6a3b6b6

many minor changes

ebecdc2

add (disabled) Qcms target

38d724e

build package rather then copying

4495a64

inferno-chromium requested a review from oliverchang July 9, 2018 06:27

oliverchang reviewed Jul 9, 2018

View reviewed changes

pdknsk added 5 commits July 13, 2018 00:25

getenv doesn't use errno

8f20e48

make paths dynamically

9175598

minor find fix

3b6c2e4

handle absolute path

9134b47

make run_minijail work

37e6c85

oliverchang approved these changes Jul 18, 2018

View reviewed changes

get required libraries automatically

ecda98f

alex approved these changes Jul 25, 2018

View reviewed changes

Remove ContentParentIPC fuzz target

89a5c31

inferno-chromium merged commit d96443b into google:master Jul 25, 2018

pdknsk deleted the firefox branch July 26, 2018 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

firefox #1546

firefox #1546

pdknsk commented Jun 21, 2018

alex left a comment

alex Jun 21, 2018

alex Jun 21, 2018

pdknsk Jun 25, 2018

alex Jun 21, 2018

pdknsk Jun 25, 2018

alex Jun 21, 2018

pdknsk commented Jun 25, 2018

pdknsk commented Jul 5, 2018 •

edited

oliverchang left a comment

oliverchang Jul 9, 2018

pdknsk Jul 13, 2018

oliverchang Jul 9, 2018

oliverchang Jul 9, 2018

choller Jul 9, 2018

choller commented Jul 9, 2018

pdknsk commented Jul 13, 2018

oliverchang left a comment

pdknsk commented Jul 20, 2018

alex left a comment

alex Jul 25, 2018

pdknsk commented Jul 26, 2018

firefox #1546

firefox #1546

Conversation

pdknsk commented Jun 21, 2018

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdknsk commented Jun 25, 2018

pdknsk commented Jul 5, 2018 • edited

oliverchang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choller commented Jul 9, 2018

pdknsk commented Jul 13, 2018

oliverchang left a comment

Choose a reason for hiding this comment

pdknsk commented Jul 20, 2018

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdknsk commented Jul 26, 2018

pdknsk commented Jul 5, 2018 •

edited