Patchwork D7106: fuzz: restrict dirs fuzzer to only 40k of input

login
register
mail settings
Submitter phabricator
Date Oct. 15, 2019, 1:58 p.m.
Message ID <differential-rev-PHID-DREV-iaavpvmvpazpy7p7wygz-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/42357/
State New
Headers show

Comments

phabricator - Oct. 15, 2019, 1:58 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Experimentally one very long path of this size shouldn't blow the RAM
  budget in the fuzzer environment, and it's not really exciting to
  discover that building the dirs dict for one stupidly long path uses a
  ton of RAM.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7106

AFFECTED FILES
  contrib/fuzz/dirs.cc

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 16, 2019, 1:55 a.m.
indygreg added a comment.


  If we blow stupid amounts of memory in the `dirs` internals, perhaps we should be enforcing a length limit in the implementation instead of burying our head in the sand by not triggering it via fuzzing.
  
  If we need to teach the fuzzer to only send small input so we don't trigger exceptions, fine. But if this is an OOM vector, I'd like to fix it. What do you think?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7106/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7106

To: durin42, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - Oct. 16, 2019, 4:16 a.m.
durin42 added a comment.


  The concern is basically that the fuzzer will figure out something dumb, like `'a/' * 40000 + 'b'` which consumes around a gig of memory. I don't think this is a serious OOM vector, since any such manifest would also be inordinately large?
  
  Maybe I'm being an optimist.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7106/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7106

To: durin42, #hg-reviewers
Cc: indygreg, mercurial-devel

Patch

diff --git a/contrib/fuzz/dirs.cc b/contrib/fuzz/dirs.cc
--- a/contrib/fuzz/dirs.cc
+++ b/contrib/fuzz/dirs.cc
@@ -35,9 +35,10 @@ 
 
 int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size)
 {
-	// Don't allow fuzzer inputs larger than 100k, since we'll just bog
-	// down and not accomplish much.
-	if (Size > 100000) {
+	// Don't allow fuzzer inputs larger than 40k: the fuzzer will
+	// discover that if it passes us one extremely long path we'll
+	// use a ton of RAM, which is a surprise to nobody.
+	if (Size > 40000) {
 		return 0;
 	}
 	PyObject *mtext =