iclelland@, your fuzzer is working! Would you mind helping to find an owner?

I'll take a look, thanks!

It looks like the fuzzer declares a stack overflow at stack depth 250 -- the JSON parsing code has a limit of 1000 built-in for protection, but that never gets exercised because the libfuzzer doesn't trust it to go that far :)

Is cluster-fuzz's stack size configurable, or do I need to change the stack depth limit in JSONParser.cpp to appease it?

Interesting. I think that ASan just detects an exhaustion of
$ ulimit -s
8192

~8 MB.

We can increase the limit, but I'm not sure that we should. CC'ing more folks.

same problem as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=132
-- asan emits much larger stack frames some times. 
So, *maybe*, increasing the stack size for asan runs to e.g. 16Mb or 24Mb is ok. 

OTOH, if the API in question is going to be used in threads other than main(),
that may have much smaller stack, some of such stack overflow cases should be treated as bugs. 

same problem as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=132
-- asan emits much larger stack frames some times. 
So, *maybe*, increasing the stack size for asan runs to e.g. 16Mb or 24Mb is ok. 

OTOH, if the API in question is going to be used in threads other than main(),
that may have much smaller stack, some of such stack overflow cases should be treated as bugs. 

I suspect that the fuzzer stack trace is being truncated after 250 items, since the outermost call in the list is to ::buildValue.

As far as I can tell from inspection, the fuzzer input shouldn't have crashed; it should have bailed out after the 1000th '['. My local testing shows that the buildValue call stack frames are 416 bytes, and if I test with the exact failing input, it uses exactly 416000 bytes of stack before refusing to parse the rest (and correctly returning failure).


I can make the max depth configurable in the parse call; certainly there's no need in this application for even 5 levels of nesting, so capping at, say 50, should be sufficient.

>> local testing shows that the buildValue call stack frames are 416 bytes
remember that asan produces larger frames. sometimes much larger. 

That's a good point -- compiled for asan, I do see 1KB stack frames locally, and I have no idea what size they are on the clusterfuzz bots. The point was just that its seemed like a genuine difference between the fuzzing environment and the actual running environment, where the crash was only triggerable on the fuzzer.

Given comment #6, it sounds like it would also be useful to limit the nesting depth to something reasonable when parsing untrusted input.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/928cd03454e06bf9833e1940aa58d014acafcf99

commit 928cd03454e06bf9833e1940aa58d014acafcf99
Author: iclelland <iclelland@chromium.org>
Date: Tue Nov 22 00:50:28 2016

Add a configurable maximum parse depth for the blink JSON parser.

BUG= 662920 

Review-Url: https://codereview.chromium.org/2517183002
Cr-Commit-Position: refs/heads/master@{#433716}

[modify] https://crrev.com/928cd03454e06bf9833e1940aa58d014acafcf99/third_party/WebKit/Source/platform/json/JSONParser.cpp
[modify] https://crrev.com/928cd03454e06bf9833e1940aa58d014acafcf99/third_party/WebKit/Source/platform/json/JSONParser.h
[modify] https://crrev.com/928cd03454e06bf9833e1940aa58d014acafcf99/third_party/WebKit/Source/platform/json/JSONParserTest.cpp

Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cb3e5ca8d303f10841d9985ff350d9fcbca49c9d

commit cb3e5ca8d303f10841d9985ff350d9fcbca49c9d
Author: iclelland <iclelland@chromium.org>
Date: Wed Nov 23 05:37:09 2016

Add a configurable parse-depth limit when parsing JFV headers, and use it for Feature-Policy Header

The limit is deliberately set higher than the actual required depth
for a valid policy, so that strings which are malformed but not
malicious can still be passed through the Feature Policy header parser
which can emit more meaningful error messages.

BUG= 662920 

Review-Url: https://codereview.chromium.org/2520403002
Cr-Commit-Position: refs/heads/master@{#434123}

[modify] https://crrev.com/cb3e5ca8d303f10841d9985ff350d9fcbca49c9d/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
[modify] https://crrev.com/cb3e5ca8d303f10841d9985ff350d9fcbca49c9d/third_party/WebKit/Source/platform/json/JSONParserFuzzer.cpp
[modify] https://crrev.com/cb3e5ca8d303f10841d9985ff350d9fcbca49c9d/third_party/WebKit/Source/platform/network/HTTPParsers.cpp
[modify] https://crrev.com/cb3e5ca8d303f10841d9985ff350d9fcbca49c9d/third_party/WebKit/Source/platform/network/HTTPParsers.h

ClusterFuzz has detected this issue as fixed in range 434104:434129.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5672738414133248

Fuzzer: libfuzzer_feature_policy_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7ffc601a4fe8
Crash State:
  blink::JSONValue::operator new
  blink::JSONArray::create
  std::__1::unique_ptr<blink::JSONValue, std::__1::default_delete<blink::JSONValue
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan_debug&range=429695:429743
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan_debug&range=434104:434129

Minimized Testcase (1.36 Kb): https://cluster-fuzz.appspot.com/download/AMIfv969OZ-LzMfeDHsF0-2m5KlCeOmJmrCciT-WWQzUtaOMvL6EUGA1gT28x9_1XDY83YyG7c-YLumVX9GVseTwMdY-DLTlIuRroicv4xIes251xq9ei7HFwLERgDIxGDd_7zkEoCv3vWqICFvuULvqgzpQb1A9Fw?testcase_id=5672738414133248

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase is verified as fixed, closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.