Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650513002:
Fix buffer overrun in PDF accessibility code. (Closed)
DescriptionFix buffer overrun in PDF accessibility code.
GetTextRunInfo scans until it finds the start of the next text run,
then increments the character index by 1 in order to scan to the end of
the text run. That was resulting in a buffer ovverun if the first scan
reached the end of the array of characters without finding a non-whitespace
character. Fix it by ensuring we never increment past the char count.
BUG=668724
Review-Url: https://codereview.chromium.org/2650513002
Cr-Commit-Position: refs/heads/master@{#447344}
Committed: https://chromium.googlesource.com/chromium/src/+/e4e4b0140002c58d760bf8d9f3fcfd7d29343486
Patch Set 1 #Patch Set 2 : Adds test #
Total comments: 1
Patch Set 3 : Depend on pdf_private #
Total comments: 2
Patch Set 4 : Fixed merge error #
Messages
Total messages: 28 (12 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmazzoni@chromium.org changed reviewers: + raymes@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lg but is it possible to add a test for this?
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Test added
Just checking that the PDF is ok to add to our repository? (I should have asked this with the other one in the data URL too) Also you may want to rename the pdf with the bug number, e.g. crbug####.pdf https://codereview.chromium.org/2650513002/diff/20001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2650513002/diff/20001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:730: IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfAccessibilityCrash) { nit: please add a link to the bug for this
On 2017/01/24 23:13:14, raymes wrote: > Just checking that the PDF is ok to add to our repository? (I should have asked > this with the other one in the data URL too) Good point. I'll have to examine the copyright. If not, I'll add a unit test instead.
Not sure what to do here. The file is publicly available on the Internet, it's a government document so probably free to redistribute, but I'm not sure if putting it in our repository implies something different about its license. No easy way to unit-test, and I don't know how to generate a PDF that would exhibit the same error. OK to apply the fix without a test, or do you have an idea of what we can do to add test coverage without a major engineering effort?
Please review this change to add the test file to pdf_private first: https://chromereviews.googleplex.com/559577013/ Then this should be ready to go.
https://codereview.chromium.org/2650513002/diff/40001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2650513002/diff/40001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:731: // Test a particular PDF encountered in the wild that encountered a crash nit: encountered a crash->triggered a crash https://codereview.chromium.org/2650513002/diff/40001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:753: } Hmm is this meant to be here?
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Sorry, merge error. Take a look.
lgtm
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485896701429110,
"parent_rev": "2ed48364ede3c10466f4d854fe6684bef63e645a", "commit_rev":
"e4e4b0140002c58d760bf8d9f3fcfd7d29343486"}
Message was sent while issue was closed.
Description was changed from ========== Fix buffer overrun in PDF accessibility code. GetTextRunInfo scans until it finds the start of the next text run, then increments the character index by 1 in order to scan to the end of the text run. That was resulting in a buffer ovverun if the first scan reached the end of the array of characters without finding a non-whitespace character. Fix it by ensuring we never increment past the char count. BUG=668724 ========== to ========== Fix buffer overrun in PDF accessibility code. GetTextRunInfo scans until it finds the start of the next text run, then increments the character index by 1 in order to scan to the end of the text run. That was resulting in a buffer ovverun if the first scan reached the end of the array of characters without finding a non-whitespace character. Fix it by ensuring we never increment past the char count. BUG=668724 Review-Url: https://codereview.chromium.org/2650513002 Cr-Commit-Position: refs/heads/master@{#447344} Committed: https://chromium.googlesource.com/chromium/src/+/e4e4b0140002c58d760bf8d9f3fc... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e4e4b0140002c58d760bf8d9f3fc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
