Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(965)

Issue 833263003: PDF: Fix extra NUL characters from incorrect WriteInto() calls. (Closed)

Created:
5 years, 11 months ago by Lei Zhang
Modified:
5 years, 11 months ago
Reviewers:
Deepak, raymes
CC:
chromium-reviews, Tom Sepez
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

PDF: Fix extra NUL characters from incorrect WriteInto() calls. BUG=445307 Committed: https://crrev.com/9ea045034e75880ad0a9c6ff13d3b7a7038c459d Cr-Commit-Position: refs/heads/master@{#310185}

Patch Set 1 #

Total comments: 2

Patch Set 2 : add comment #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -13 lines) Patch
M pdf/pdfium/pdfium_engine.cc View 1 1 chunk +12 lines, -4 lines 0 comments Download
M pdf/pdfium/pdfium_page.cc View 2 chunks +2 lines, -2 lines 3 comments Download
M pdf/pdfium/pdfium_range.cc View 1 chunk +11 lines, -7 lines 0 comments Download

Messages

Total messages: 13 (2 generated)
Lei Zhang
Please review carefully, as some APIs take the number of characters while others take the ...
5 years, 11 months ago (2015-01-06 03:56:40 UTC) #2
raymes
lgtm https://codereview.chromium.org/833263003/diff/1/pdf/pdfium/pdfium_range.cc File pdf/pdfium/pdfium_range.cc (right): https://codereview.chromium.org/833263003/diff/1/pdf/pdfium/pdfium_range.cc#newcode72 pdf/pdfium/pdfium_range.cc:72: // NUL character by calling resize(). Not sure ...
5 years, 11 months ago (2015-01-06 05:43:24 UTC) #3
raymes
lgtm
5 years, 11 months ago (2015-01-06 05:43:24 UTC) #4
Lei Zhang
https://codereview.chromium.org/833263003/diff/1/pdf/pdfium/pdfium_range.cc File pdf/pdfium/pdfium_range.cc (right): https://codereview.chromium.org/833263003/diff/1/pdf/pdfium/pdfium_range.cc#newcode72 pdf/pdfium/pdfium_range.cc:72: // NUL character by calling resize(). On 2015/01/06 05:43:24, ...
5 years, 11 months ago (2015-01-06 22:53:03 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/833263003/20001
5 years, 11 months ago (2015-01-06 22:53:44 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 11 months ago (2015-01-07 00:26:52 UTC) #8
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/9ea045034e75880ad0a9c6ff13d3b7a7038c459d Cr-Commit-Position: refs/heads/master@{#310185}
5 years, 11 months ago (2015-01-07 00:27:41 UTC) #9
raymes
Let me know what you think. I'm happy to create a patch. Thanks! https://codereview.chromium.org/833263003/diff/20001/pdf/pdfium/pdfium_page.cc File ...
5 years, 11 months ago (2015-01-14 00:13:12 UTC) #10
Lei Zhang
https://codereview.chromium.org/833263003/diff/20001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/833263003/diff/20001/pdf/pdfium/pdfium_page.cc#newcode411 pdf/pdfium/pdfium_page.cc:411: reinterpret_cast<unsigned short*>(WriteInto(&url, url_length + 1)); On 2015/01/14 00:13:12, raymes ...
5 years, 11 months ago (2015-01-14 01:21:54 UTC) #11
Deepak
https://codereview.chromium.org/833263003/diff/20001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/833263003/diff/20001/pdf/pdfium/pdfium_page.cc#newcode411 pdf/pdfium/pdfium_page.cc:411: reinterpret_cast<unsigned short*>(WriteInto(&url, url_length + 1)); On 2015/01/14 01:21:54, Lei ...
5 years, 11 months ago (2015-01-14 03:50:38 UTC) #12
Lei Zhang
5 years, 11 months ago (2015-01-14 05:29:42 UTC) #13
Message was sent while issue was closed.
On 2015/01/14 03:50:38, Deepak wrote:
> https://codereview.chromium.org/833263003/diff/20001/pdf/pdfium/pdfium_page.cc
> File pdf/pdfium/pdfium_page.cc (right):
> 
>
https://codereview.chromium.org/833263003/diff/20001/pdf/pdfium/pdfium_page.c...
> pdf/pdfium/pdfium_page.cc:411: reinterpret_cast<unsigned
short*>(WriteInto(&url,
> url_length + 1));
> On 2015/01/14 01:21:54, Lei Zhang wrote:
> > On 2015/01/14 00:13:12, raymes wrote:
> > > I just had another look at this and I think that we should keep the > 1
> above
> > > and remove the + 1 here. The reason is that FPDFLink_GetURL seems to
return
> a
> > > value which /includes/ the null terminator. WDYT?
> > 
> > Gah! I will make another CL...
> 
> I personally don't think that FPDFLink_GetURL() gives me len that have null
> character included.
> as 
> int len= cbUTF16URL.GetLength()/sizeof(unsigned short);
> This does not include null terminator.
> with the previous code we just need to add null terminator in the last, as we
> are passing 1 extra in the url_length.

Next CL is here: https://codereview.chromium.org/848073003/

Let's continue the conversation there. I added DCHECKs in that CL to verify we
get back a null terminator...

Powered by Google App Engine
This is Rietveld 408576698