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

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

BUG=445307
Committed: https://crrev.com/9ea045034e75880ad0a9c6ff13d3b7a7038c459d
Cr-Commit-Position: refs/heads/master@{#310185}

[Lei Zhang, 2015-01-06 03:56:39]

thestig@chromium.org changed reviewers:
+ deepak.m1@samsung.com, raymes@chromium.org

[Lei Zhang, 2015-01-06 03:56:40]

Please review carefully, as some APIs take the number of characters while others
take the number of bytes. @_@

[raymes, 2015-01-06 05:43:24]

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#n...
pdf/pdfium/pdfium_range.cc:72: // NUL character by calling resize().
Not sure if you want to put this comment above your other similar change in
pdfium_engine.cc as well, it's a bit subtle.

In addition to the comment, it also seems like FPDF_GetText could write fewer
characters than specified, in which case you would also want to resize as well.

It's tricky to get a string out of the API!

[raymes, 2015-01-06 05:43:24]

lgtm

[Lei Zhang, 2015-01-06 22:53:03]

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#n...
pdf/pdfium/pdfium_range.cc:72: // NUL character by calling resize().
On 2015/01/06 05:43:24, raymes wrote:
> Not sure if you want to put this comment above your other similar change in
> pdfium_engine.cc as well, it's a bit subtle.
> 
> In addition to the comment, it also seems like FPDF_GetText could write fewer
> characters than specified, in which case you would also want to resize as
well.
> 
> It's tricky to get a string out of the API!

Comment added. Some places call len = Foo(NULL_buffer, 0) to get the byte/char
count, then we use it immediately in Foo(buffer, len). In both of these
FPDF_GetText() calls, we have a character count that did not originate from
FPDF_GetText(), so we definitely should check the return value.

[Lei Zhang, 2015-01-06 22:53:06]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-01-06 22:53:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/833263003/20001

[commit-bot: I haz the power, 2015-01-07 00:26:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-07 00:27:41]

Patchset 2 (id:??) landed as
https://crrev.com/9ea045034e75880ad0a9c6ff13d3b7a7038c459d
Cr-Commit-Position: refs/heads/master@{#310185}

[raymes, 2015-01-14 00:13:12]

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 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));
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?

[Lei Zhang, 2015-01-14 01:21:54]

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

[Deepak, 2015-01-14 03:50:38]

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.

[Lei Zhang, 2015-01-14 05:29:42]

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