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

Issue 912783 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
hobby only
Closed: Dec 8
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Out-of-memory in csv_reader_fuzzer

Project Member Reported by ClusterFuzz, Dec 7

Issue description

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

Fuzzer: libFuzzer_csv_reader_fuzzer
Fuzz target binary: csv_reader_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Out-of-memory (exceeds 2048 MB)
Crash Address: 
Crash State:
  csv_reader_fuzzer
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=604452:604462

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Dec 7

Labels: OS-Chrome OS-Windows OS-Mac
Project Member

Comment 2 by ClusterFuzz, Dec 7

Cc: vabr@chromium.org
Labels: ClusterFuzz-Auto-CC
Automatically adding ccs based on OWNERS file / target commit history.

If this is incorrect, please add ClusterFuzz-Wrong label.
Cc: -vabr@chromium.org battre@google.com jdoerrie@chromium.org
Components: UI>Browser>Passwords
Labels: -Pri-1 Pri-3
Owner: vabr@chromium.org
Status: Started (was: Untriaged)
I have not looked in where the memory is spent in the testcase, but will try to propose reasonable per-string etc. limits once I do.
It would be better if there were no overall cap on the number of password entries, but in the worst case, we might add it to avoid DDoS-ing.

The good point is that this is only used for importing passwords, which is only behind a flag (and broken) and likely to stay this way. So there is no urgency fixing this.

I'm adding battre@, the lead of the password manager team for information, in case the team restarted the import project soon.
Also adding jdoerrie@ who I expect to bother with code reviews.
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 8

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

commit f74ca2830bd37c48481207c67dbdd8952e38f54a
Author: Vaclav Brozek <vabr@chromium.org>
Date: Sat Dec 08 07:58:41 2018

Use StringPiece to save memory for CSV parsing results

The CSV parsing routine from
components/password_manager/core/browser/import/csv_reader.h stores
the parsed data in a vector of string values from the first row, and a
vector of maps with keys from that first row.

So far, the keys have been std::string, which is rather inefficient,
because it stores the same value N times, for N being the # of rows in
the CSV file, while it could have been just stored once.

Therefore, this CL changes the map keys to base::StringPiece. This
also fixes the out-of-memory issue reported by the fuzzer in the
associated bug (this was 100% reproducible before the CL).

To make this change safe, the CL also encapsulates the vectors with
parsed data in a class, so that the StringPiece does not end up
dangling if the values from the first row have been destroyed.

The CL does not rename the csv_reader.* files to match the new class
name, to reduce churn. This can be done in a separate CL if required.

Bug:  912783 
Change-Id: I44bfbbdfe8125a88f80d0d655829519c2f0d4121
Reviewed-on: https://chromium-review.googlesource.com/c/1368106
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614960}
[modify] https://crrev.com/f74ca2830bd37c48481207c67dbdd8952e38f54a/components/password_manager/core/browser/export/password_csv_writer_unittest.cc
[modify] https://crrev.com/f74ca2830bd37c48481207c67dbdd8952e38f54a/components/password_manager/core/browser/import/csv_reader.cc
[modify] https://crrev.com/f74ca2830bd37c48481207c67dbdd8952e38f54a/components/password_manager/core/browser/import/csv_reader.h
[modify] https://crrev.com/f74ca2830bd37c48481207c67dbdd8952e38f54a/components/password_manager/core/browser/import/csv_reader_unittest.cc
[modify] https://crrev.com/f74ca2830bd37c48481207c67dbdd8952e38f54a/components/password_manager/core/browser/import/password_csv_reader.cc
[modify] https://crrev.com/f74ca2830bd37c48481207c67dbdd8952e38f54a/components/password_manager/core/browser/import/password_csv_reader.h

Status: Fixed (was: Started)
Project Member

Comment 7 by ClusterFuzz, Dec 8

ClusterFuzz has detected this issue as fixed in range 614959:614960.

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

Fuzzer: libFuzzer_csv_reader_fuzzer
Fuzz target binary: csv_reader_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Out-of-memory (exceeds 2048 MB)
Crash Address: 
Crash State:
  csv_reader_fuzzer
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=604452:604462
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=614959:614960

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

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 8 by ClusterFuzz, Dec 8

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 5163314629574656 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