PDF: Yet another stab at getting WriteInto() buffer sizes correct.

PDF: Yet another stab at getting WriteInto() buffer sizes correct.

- Fix remaining case with extra null-character.
- Do not assume strings are null-terminated, even though they most likely are.
- Fix accessibility code that checked for the extra null-character.

Committed: https://crrev.com/026f6359bf17043fc7404354a1777bb368534610
Cr-Commit-Position: refs/heads/master@{#311715}

[Lei Zhang, 2015-01-14 03:14:17]

thestig@chromium.org changed reviewers:
+ raymes@chromium.org

[Lei Zhang, 2015-01-14 03:14:48]

As before, look carefully.

[raymes, 2015-01-14 05:05:52]

OK! This definitely looks right to me now :)

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.cc
File pdf/pdfium/pdfium_page.cc (right):

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.c...
pdf/pdfium/pdfium_page.cc:203: DCHECK(targets[i].url[targets[i].url.size() - 1]
!= '\0');
just double checking: does DCHECK ever evaluate to an empty string?

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.c...
pdf/pdfium/pdfium_page.cc:329: void* data = WriteInto(&target->url, buffer_size
+ 1);
:( you're right we need to be careful that the API call doesn't write the NULL
terminator into unmanaged memory.

We could have some scoped thingy which wraps WriteInto and does the resize on
destruction. WDYT?

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.c...
pdf/pdfium/pdfium_page.cc:331: engine_->doc(), action, data, buffer_size);
In this case can't we just DCHECK that buffer_size == bytes_written

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.c...
pdf/pdfium/pdfium_page.cc:427: int actual_length = FPDFLink_GetURL(links, i,
data, url_length);
in this case again I think it may be sufficient to DCHECK that this equals the
length the API returned earlier.

[Lei Zhang, 2015-01-15 00:42:08]

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.cc
File pdf/pdfium/pdfium_page.cc (right):

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.c...
pdf/pdfium/pdfium_page.cc:203: DCHECK(targets[i].url[targets[i].url.size() - 1]
!= '\0');
On 2015/01/14 05:05:52, raymes wrote:
> just double checking: does DCHECK ever evaluate to an empty string?

Sorry, having a bit of trouble parsing the question. We don't go out of bounds
due to the empty() check above if that's what you are asking.

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.c...
pdf/pdfium/pdfium_page.cc:329: void* data = WriteInto(&target->url, buffer_size
+ 1);
On 2015/01/14 05:05:52, raymes wrote:
> :( you're right we need to be careful that the API call doesn't write the NULL
> terminator into unmanaged memory.
> 
> We could have some scoped thingy which wraps WriteInto and does the resize on
> destruction. WDYT?

I'm not sure where to put it. It could also just be a TrimNullTerminator()
function.

Also, for FPDFText_GetText(), there's no DCHECK against the expected size, but
here we have an expected size.

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.c...
pdf/pdfium/pdfium_page.cc:331: engine_->doc(), action, data, buffer_size);
On 2015/01/14 05:05:52, raymes wrote:
> In this case can't we just DCHECK that buffer_size == bytes_written

Sure. I suppose the if condition should always be true.

[raymes, 2015-01-15 01:22:58]

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.cc
File pdf/pdfium/pdfium_page.cc (right):

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.c...
pdf/pdfium/pdfium_page.cc:203: DCHECK(targets[i].url[targets[i].url.size() - 1]
!= '\0');
I was just checking that it's ok to omit the {} from the if-statement in this
case where the DCHECK macro is. I think it is.

https://codereview.chromium.org/848073003/diff/20001/pdf/pdfium/pdfium_page.c...
pdf/pdfium/pdfium_page.cc:329: void* data = WriteInto(&target->url, buffer_size
+ 1);
I guess I was thinking something like what's below. But it might be overkill to
do this. I'll leave it up to you - I'm ok either way.

APIStringWriter writer(buffer_size); // buffer size includes null bytes that may
get written into.
void* data = writer.data();
...
// GetString optionally takes the size to resize to before returning.
std::string* str = writer.GetString();

[Lei Zhang, 2015-01-15 03:41:03]

I added a class. See patch set 4.

[raymes, 2015-01-15 04:33:24]

lgtm! Thanks for doing this - there are 2 CLs under review currently that need
to do the same thing, so it will be useful there too.

https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st...
File pdf/pdfium/pdfium_api_string_buffer_adapter.cc (right):

https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st...
pdf/pdfium/pdfium_api_string_buffer_adapter.cc:16: template <typename
STRING_TYPE>
nit: most template style I see in Chrome tends to use camel case for the params

https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st...
File pdf/pdfium/pdfium_api_string_buffer_adapter.h (right):

https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st...
pdf/pdfium/pdfium_api_string_buffer_adapter.h:31: // hold |expected_size_| + 1
characters, so teh PDFium API that uses the
nit: teh->the

[Lei Zhang, 2015-01-15 04:44:36]

On 2015/01/15 04:33:24, raymes wrote:
> lgtm! Thanks for doing this - there are 2 CLs under review currently that need
> to do the same thing, so it will be useful there too.
> 
>
https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st...
> File pdf/pdfium/pdfium_api_string_buffer_adapter.cc (right):
> 
>
https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st...
> pdf/pdfium/pdfium_api_string_buffer_adapter.cc:16: template <typename
> STRING_TYPE>
> nit: most template style I see in Chrome tends to use camel case for the
params

We are not at all consistent here, but CamelCase it is. I followed
string_piece.h FWIW.

>
https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st...
> File pdf/pdfium/pdfium_api_string_buffer_adapter.h (right):
> 
>
https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st...
> pdf/pdfium/pdfium_api_string_buffer_adapter.h:31: // hold |expected_size_| + 1
> characters, so teh PDFium API that uses the
> nit: teh->the

Done.

[Lei Zhang, 2015-01-15 09:06:14]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-01-15 09:07:12]

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

[commit-bot: I haz the power, 2015-01-15 09:32:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-15 09:32:45]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Lei Zhang, 2015-01-15 19:32:04]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-01-15 19:33:06]

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

[commit-bot: I haz the power, 2015-01-15 19:51:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-15 19:52:50]

Patchset 5 (id:??) landed as
https://crrev.com/026f6359bf17043fc7404354a1777bb368534610
Cr-Commit-Position: refs/heads/master@{#311715}