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

Issue 798764 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Stack-overflow in policy::RegistryDict::~RegistryDict

Project Member Reported by ClusterFuzz, Jan 3 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5838889758752768

Fuzzer: libFuzzer_preg_parser_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7ffc35870ff0
Crash State:
  policy::RegistryDict::~RegistryDict
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=518239:518261

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5838889758752768

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Jan 3 2018

Components: Enterprise
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Cc: thestig@chromium.org pmarko@chromium.org brajkumar@chromium.org phweiss@chromium.org rsesek@chromium.org
Labels: M-64 CF-NeedsTriage
Predator has provided 4 possible suspects but not sure about the culprit CL

1. Fix policy histogram PRESUBMIT by pmarko@chromium.org
2. Fix else-if after returns/breaks in components/. by thestig@chromium.org
3. New Device policy to disallow ARC on unaffiliated users by phweiss@google.com
4. Cull registered-but-unused crash keys from the registration lists. by rsesek@chromium.org

cc'ing the above suspects owners for further triage.

Thanks!
Owner: ljusten@chromium.org
Status: Assigned (was: Untriaged)
I reproduced this and looked at it with gdb, and I don't think it is related to one listed CLs. The responsible fuzzer is here:

https://cs.chromium.org/chromium/src/components/policy/core/common/preg_parser_fuzzer.cc?rcl=3659b506db01869ee2428ab5585646144309daea&l=36

It seems that the fuzzer provides data for a RegistryDict that is nested 36892 times (that is how often a breakpoint was hit during the construction of one new directory). For construction, this is not a problem, but as soon as the destructor of the root dict is called, we get that a multiple of that many nested function calls, resulting in the stack overflow.

I'm assigning to ljusten, who wrote the fuzzer and some of the related code.
Cc: -thestig@chromium.org
Not related to my CL.
Thanks for the analysis Philipp.

Lutz, sounds like an iteration limit in HandleRecord's registry path handling (preg_parser.cc) would fix this?
Thanks for taking a look. Seems like we're bailing during a std::map destruction, so there's not much we can do there and limiting the stack depth seems like the best option. In practice, the depth shouldn't be much greater than 10, so a limit of 1024 should be plenty.

Still a bit surprised that std::map fails that early, but I guess it makes sense with the default stack size (1MB?).
Note that there are probably multiple nested function calls involved for each RegistryDict nest level, as the structure looks somewhat like
RegistryDict -> map(keys_) -> std::pair -> unique_ptr -> RegistryDict
(+ whatever map uses internally).
@early fail: The stacktrace on clusterfuzz.com is cut-off. On my machine, the stacktrace on the crash actually has a six-digit length. Here are the last 9 lines:

#261746 destroy<std::__1::pair<std::__1::basic_string<char> const, std::__1::unique_ptr<policy::RegistryDict, std::__1::default_delete<policy::RegistryDict> > > > (__p=0x7040001c67b8, __a=...)
    at ../../buildtools/third_party/libc++/trunk/include/memory:1550
#261747 std::__1::__tree<std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<policy::RegistryDict, std::__1::default_delete<policy::RegistryDict> > >, std::__1::__map_value_compare<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<policy::RegistryDict, std::__1::default_delete<policy::RegistryDict> > >, policy::CaseInsensitiveStringCompare, true>, std::__1::allocator<std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<policy::RegistryDict, std::__1::default_delete<policy::RegistryDict> > > > >::destroy (this=<optimized out>, __nd=0x704000000040)
    at ../../buildtools/third_party/libc++/trunk/include/__tree:1833
#261748 0x00000000004bb1a4 in std::__1::__tree<std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<policy::RegistryDict, std::__1::default_delete<policy::RegistryDict> > >, std::__1::__map_value_compare<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<policy::RegistryDict, std::__1::default_delete<policy::Regis---Type <return> to continue, or q <return> to quit---
tryDict> > >, policy::CaseInsensitiveStringCompare, true>, std::__1::allocator<std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<policy::RegistryDict, std::__1::default_delete<policy::RegistryDict> > > > >::clear (this=<optimized out>)
    at ../../buildtools/third_party/libc++/trunk/include/__tree:1870
#261749 0x00000000004b5846 in policy::RegistryDict::~RegistryDict (
    this=0x7fffffffbe60)
    at ../../components/policy/core/common/registry_dict.cc:158
#261750 0x00000000004933c9 in LLVMFuzzerTestOneInput (
    data=0x7ffff7f52000 "PReg\001", size=151524)
    at ../../components/policy/core/common/preg_parser_fuzzer.cc:42
#261751 0x00000000004fc92c in fuzzer::Fuzzer::ExecuteCallback (
    this=0x713000000000, Data=0x7ffff7f78000 "PReg\001", Size=151524)
    at ../../third_party/libFuzzer/src/FuzzerLoop.cpp:515
#261752 0x00000000004bea3f in fuzzer::RunOneTest (F=0x713000000000, 
    InputFilePath=<optimized out>, MaxLen=<optimized out>)
    at ../../third_party/libFuzzer/src/FuzzerDriver.cpp:280
#261753 0x00000000004ccca1 in fuzzer::FuzzerDriver (argc=0x702076000001, 
    argv=0x0, Callback=0x703000000600)
    at ../../third_party/libFuzzer/src/FuzzerDriver.cpp:703
#261754 0x00000000005239e1 in main (argc=6, argv=0x7fffffffd948)
    at ../../third_party/libFuzzer/src/FuzzerMain.cpp:20

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 18 2018

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

commit 7fe21736cde8fc94a1f60c3ccb9c551c4f5e2992
Author: Pavol Marko <pmarko@chromium.org>
Date: Thu Jan 18 09:22:38 2018

Limit nesting of RegistryDicts in preg_parser

Limit number of path components in preg_parser and thus nesting of the
resulting RegistryDict tree.
Also switch to SplitStringPiece to use less memory.

Bug:  798764 
Test: clusterfuzz repro and components_unittests --gtest_filter=PRegParserTest*
Change-Id: I2a47832240f1a61dc7877d8086bba0b3347d3055
Reviewed-on: https://chromium-review.googlesource.com/870033
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530096}
[modify] https://crrev.com/7fe21736cde8fc94a1f60c3ccb9c551c4f5e2992/components/policy/core/common/preg_parser.cc

Project Member

Comment 10 by ClusterFuzz, Jan 19 2018

ClusterFuzz has detected this issue as fixed in range 529955:530274.

Detailed report: https://clusterfuzz.com/testcase?key=5838889758752768

Fuzzer: libFuzzer_preg_parser_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7ffc35870ff0
Crash State:
  policy::RegistryDict::~RegistryDict
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=518239:518261
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=529955:530274

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5838889758752768

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.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 11 by ClusterFuzz, Jan 19 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 5838889758752768 is verified as fixed, so closing issue as verified.

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

Sign in to add a comment