New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 648076 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Stack-overflow in Parser::ParseExpression

Project Member Reported by ClusterFuzz, Sep 18 2016

Issue description

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

Fuzzer: gn_parser_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7fffeba78d80
Crash State:
  Parser::ParseExpression
  ParseExpression
  Parser::Group
  

Minimized Testcase (3.69 Kb): https://cluster-fuzz.appspot.com/download/AMIfv961kfkjAV3bYjdiC6ZD34UxCxqMQjc4_01M12xpY4bKJLz7ySh6_x2S1WDB1nN6jrjF7WLk7PgNWccWRd-1hnWvSprnTwlLcbizcrnfplrj4oniJ0hAxy6MMlNUSmaDZcddgC4woglu8mIMZyXNkbiF3DP1Kw?testcase_id=6027139944284160

Issue manually filed by: mmoroz

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

Comment 1 by mmoroz@chromium.org, Sep 18 2016

Cc: mmoroz@chromium.org kcc@chromium.org aizatsky@chromium.org
Labels: Build-Tools-GN
Owner: thakis@chromium.org
Looks similar to  bug 641454 , but this one is reproducible.

Nico, could you please help to find an owner?
Project Member

Comment 2 by ClusterFuzz, Sep 18 2016

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

Fuzzer: libfuzzer_gn_parser_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7fff0fbb3f58
Crash State:
  Parser::ParseExpression
  Parser::ParseExpression
  Parser::Group
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan_debug&range=414095:414176

Minimized Testcase (1.24 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95Rj5fIOHpjZIqq0Y3OZmeZ7HQp1Ww-ZaShxUz3Cc82DcjCMyBh-FtsMwAx63CTeKvMUGHesUge99FCfwuJYLDAhO1OJvaO1X36WdErPivRJcv8tIJH3yZAR3a-i9JT62ZBoeu3W8EF2jtSBWmVohuG9LvLlQ?testcase_id=4958105924534272

Additional requirements: Requires Gestures

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

Comment 3 by thakis@chromium.org, Sep 19 2016

 bug 641454  is similar: gn uses a recursive-descent parser, and giving a deeply-enough nested file, it'll stack overflow.  In practice, that's ok. The file that libfuzzer found is 124 nested ifs with some other stuff thrown in -- that just doesn't happen in practice. (In  bug 641454  I made gn need less stack memory per level of recursion, which meant the fuzzer needed longer to find this again, but it the problem didn't go away).

Do you have any suggestions on how to deal with this? The only thing that's going to write files nested this deep is libfuzzer. We could add a max nesting depth to gn, but that'd be an arbitrary limit, and it'd make the code more complicated solely for libfuzzer, so that doesn't sound so great to me.

Comment 4 by mmoroz@google.com, Sep 28 2016

Can we measure the depth anyhow in parser_fuzzer.cc? May be count nested blocks in a simple way? Would be great if we could check the depth against some number (e.g. 100) and return 0 in the fuzzer source code.

Comment 5 by thakis@chromium.org, Sep 30 2016

But that adds a fairly arbitrary limit to max nesting depth, while as-is things can be arbitrarily nested as long as the stack is large enough.  In this case I don't think it'll ever be a problem in practice, but adding arbitrary limitations to programs feels a bit weird. (I think I remember reading somewhere -- some old GNU manifesto -- that hardcoded limits should be avoided; I think that was motivated since people had to recompile tex to increase constants for larger documents all the time back in the day.)
I totally agree with you.

Though sometimes we have to hack the things in fuzzers to keep them effective, for example:

- ignore REGEXP statement in sqlite3 fuzzer (it has too many "valid" usages that break the fuzzing): https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/sqlite3_prepare_v2_fuzzer.cc?l=41
- avoid big images in libpng fuzzer (too slow and can go out-of-memory): https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc?l=91

Adding some simple logic to the fuzzer (e.g. if there are more than 100 '{' characters in the data, then return 0) looks like a reasonable trade-off to me. I believe that we still can reach really good coverage and also will not crash on irrelevant issues.
Project Member

Comment 7 by ClusterFuzz, Oct 7 2016

ClusterFuzz has detected this issue as fixed in range 423769:423794.

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

Fuzzer: gn_parser_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7fffeba78d80
Crash State:
  Parser::ParseExpression
  ParseExpression
  Parser::Group
  
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan&range=423769:423794

Minimized Testcase (3.69 Kb): https://cluster-fuzz.appspot.com/download/AMIfv961kfkjAV3bYjdiC6ZD34UxCxqMQjc4_01M12xpY4bKJLz7ySh6_x2S1WDB1nN6jrjF7WLk7PgNWccWRd-1hnWvSprnTwlLcbizcrnfplrj4oniJ0hAxy6MMlNUSmaDZcddgC4woglu8mIMZyXNkbiF3DP1Kw?testcase_id=6027139944284160

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.
Project Member

Comment 8 by ClusterFuzz, Oct 8 2016

ClusterFuzz has detected this issue as fixed in range 423769:423794.

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

Fuzzer: gn_parser_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7fffeba78d80
Crash State:
  Parser::ParseExpression
  ParseExpression
  Parser::Group
  
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan&range=423769:423794

Minimized Testcase (3.69 Kb): https://cluster-fuzz.appspot.com/download/AMIfv961kfkjAV3bYjdiC6ZD34UxCxqMQjc4_01M12xpY4bKJLz7ySh6_x2S1WDB1nN6jrjF7WLk7PgNWccWRd-1hnWvSprnTwlLcbizcrnfplrj4oniJ0hAxy6MMlNUSmaDZcddgC4woglu8mIMZyXNkbiF3DP1Kw?testcase_id=6027139944284160

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.
Status: Fixed (was: Untriaged)
Marking the issue as Fixed as ClusterFuzz detected the issue as fixed as provided in per Comment 7 & 8.
Thank You.
Project Member

Comment 10 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
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
Project Member

Comment 11 by ClusterFuzz, Dec 16 2016

ClusterFuzz has detected this issue as fixed in range 438794:438815.

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

Fuzzer: libfuzzer_gn_parser_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7fff0fbb3f58
Crash State:
  Parser::ParseExpression
  Parser::ParseExpression
  Parser::Group
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan_debug&range=414095:414176
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan_debug&range=438794:438815

Minimized Testcase (1.24 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95Rj5fIOHpjZIqq0Y3OZmeZ7HQp1Ww-ZaShxUz3Cc82DcjCMyBh-FtsMwAx63CTeKvMUGHesUge99FCfwuJYLDAhO1OJvaO1X36WdErPivRJcv8tIJH3yZAR3a-i9JT62ZBoeu3W8EF2jtSBWmVohuG9LvLlQ?testcase_id=4958105924534272

Additional requirements: Requires Gestures

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.
Cc: brettw@chromium.org
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 30 2017

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

commit 697f302d91e7ad4466416ab22c290c7fea9c6b93
Author: Penny MacNeil <pennymac@chromium.org>
Date: Mon Oct 30 21:29:41 2017

[GN fuzzer] Stack overflow fix.

Fuzzathon 2017.

Bug:  648076 , 749793 , 773426 , 768111 , 754972 , 734401 , 734200 
Change-Id: Ic608c5a374252809443a879ad4e2ddf8f6184697
Reviewed-on: https://chromium-review.googlesource.com/736159
Commit-Queue: Penny MacNeil <pennymac@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512626}
[modify] https://crrev.com/697f302d91e7ad4466416ab22c290c7fea9c6b93/tools/gn/parser_fuzzer.cc

Sign in to add a comment