Speculative fix for uninitialized value in CFX_ByteString().

Speculative fix for uninitialized value in CFX_ByteString().

If somehow different length values could be obtained by two successive calls
to Doc_getFilePath() (and FieldBrowse() for that matter), and the method is
true to the API documentation that says "The return value always indicated
number of bytes required for the buffer, even when there is no buffer
specified, or the buffer size is less then required", then it is possible
to get a returned length describing memory beyond the current buffer.

We can make the corresponding JS_docGetFilePath() method more robust against
this case by applying better checks to the returned value.

This probably is unrelated since ASAN seems to be flagging the corresponding bug
as UAF, but doesn't hurt to make things more robust.

BUG=392956
R=jun_fang@foxitsoftware.com

Committed: https://pdfium.googlesource.com/pdfium/+/0d3b5cc

[Tom Sepez, 2014-07-29 22:13:38]

Jun, please review.

I'm not sure about the ActualLength -1 usage.

The API comment says "Number of bytes the filePath consumes, including trailing
zeros."

Yet the pdfium engine is returning std::string::size(), which doesn't count the
trailing NUL.

So is comment wrong again? If the path is "foo", do we return 3 or 4?

[jun_fang, 2014-07-30 00:37:54]

On 2014/07/29 22:13:38, Tom Sepez wrote:
> Jun, please review.
> 
> I'm not sure about the ActualLength -1 usage.
> 
> The API comment says "Number of bytes the filePath consumes, including
trailing
> zeros."
> 
> Yet the pdfium engine is returning std::string::size(), which doesn't count
the
> trailing NUL.
> 
> So is comment wrong again? If the path is "foo", do we return 3 or 4?


This function is callback function. It depends on the implementation in
applications.
So we can't say that it's correct or wrong. BTW, when I checked the related
code,
I found an error in the following code: 

"pData->m_String[nLen] = 0;" should be changed to "pData->m_String[nLen] =
'\0';"

static CFX_StringData* FX_AllocString(int nLen)
{
    // |nLen| is currently declared as in |int|. TODO(palmer): It should be
    // a |size_t|, or at least unsigned.
    if (nLen == 0 || nLen < 0) {
        return NULL;
    }
    base::CheckedNumeric<int> nSize = nLen;
    nSize += sizeof(long) * 3 + 1;
    CFX_StringData* pData = (CFX_StringData*)FX_Alloc(FX_BYTE,
nSize.ValueOrDie());
    if (!pData) {
        return NULL;
    }
    pData->m_nAllocLength = nLen;
    pData->m_nDataLength = nLen;
    pData->m_nRefs = 1;
    pData->m_String[nLen] = 0;
    return pData;
}

[jun_fang, 2014-07-30 00:38:01]

https://codereview.chromium.org/423233002/diff/40001/fpdfsdk/include/fsdk_mgr.h
File fpdfsdk/include/fsdk_mgr.h (right):

https://codereview.chromium.org/423233002/diff/40001/fpdfsdk/include/fsdk_mgr...
fpdfsdk/include/fsdk_mgr.h:206: if(nRequiredLen <= 0)
Need to add a blank here.

[Tom Sepez, 2014-07-30 16:47:41]

> This function is callback function. It depends on the implementation in
> applications. So we can't say that it's correct or wrong.
Alright, I'll leave the length caclulation as-is and not subtract one.

> I found an error in the following code: 
> 
> "pData->m_String[nLen] = 0;" should be changed to "pData->m_String[nLen] =
> '\0';"
> 
I wouldn't call this a bug, perhaps a style violation.  0 is a perfectly fine
zero to use for uses of zero.

[Tom Sepez, 2014-07-30 16:47:54]

On 2014/07/30 00:38:01, jun_fang wrote:
>
https://codereview.chromium.org/423233002/diff/40001/fpdfsdk/include/fsdk_mgr.h
> File fpdfsdk/include/fsdk_mgr.h (right):
> 
>
https://codereview.chromium.org/423233002/diff/40001/fpdfsdk/include/fsdk_mgr...
> fpdfsdk/include/fsdk_mgr.h:206: if(nRequiredLen <= 0)
> Need to add a blank here.
Right. Done.

[jun_fang, 2014-07-30 16:54:14]

On 2014/07/30 16:47:41, Tom Sepez wrote:
> > This function is callback function. It depends on the implementation in
> > applications. So we can't say that it's correct or wrong.
> Alright, I'll leave the length caclulation as-is and not subtract one.
> 
> > I found an error in the following code: 
> > 
> > "pData->m_String[nLen] = 0;" should be changed to "pData->m_String[nLen] =
> > '\0';"
> > 
> I wouldn't call this a bug, perhaps a style violation.  0 is a perfectly fine
> zero to use for uses of zero.

If you check the related code, you will find the writer use
pData->m_String[nLen] 
as trailing char. So 0 is totally different with '\0' in ascii code.

[Tom Sepez, 2014-07-30 16:55:15]

> So 0 is totally different with '\0' in ascii code.

No it isn't.  What are you talking about?

[Tom Sepez, 2014-07-30 16:57:18]

On 2014/07/30 16:55:15, Tom Sepez wrote:
> > So 0 is totally different with '\0' in ascii code.
> 
> No it isn't.  What are you talking about?

'0' is different than '\0', but 0 == '\0'.

[jun_fang, 2014-07-30 17:01:35]

On 2014/07/30 16:55:15, Tom Sepez wrote:
> > So 0 is totally different with '\0' in ascii code.
> 
> No it isn't.  What are you talking about?

Please see the following code:

static CFX_StringData* FX_AllocString(int nLen)
{
    // |nLen| is currently declared as in |int|. TODO(palmer): It should be
    // a |size_t|, or at least unsigned.
    if (nLen == 0 || nLen < 0) {
        return NULL;
    }
    base::CheckedNumeric<int> nSize = nLen;
    nSize += sizeof(long) * 3 + 1;
    CFX_StringData* pData = (CFX_StringData*)FX_Alloc(FX_BYTE,
nSize.ValueOrDie());
    if (!pData) {
        return NULL;
    }
    pData->m_nAllocLength = nLen;
    pData->m_nDataLength = nLen;
    pData->m_nRefs = 1;
    pData->m_String[nLen] = 0;
    return pData;
}

This function is called at line 220 of fpdfsdk/include/fsdk_mgr.h. The writer
wants to set a trailing char in the end of string by using
'pData->m_String[nLen] = 0'. but it doesn't work because 0 is not '\0'.

[Tom Sepez, 2014-07-30 17:07:40]

> but it doesn't work because 0 is not '\0'.
Wrong.  0 and '\0' have the same value. 0. Always have. You are confused.

[jun_fang, 2014-07-30 17:12:18]

On 2014/07/30 17:07:40, Tom Sepez wrote:
> > but it doesn't work because 0 is not '\0'.
> Wrong.  0 and '\0' have the same value. 0. Always have. You are confused.

Are you sure? 0 has the ascii value 48 but '\0' has the ascii value 0. They are
different.

[Tom Sepez, 2014-07-30 17:13:50]

Are you sure? 0 has the ascii value 48 but '\0' has the ascii value 0. They are
> different.

0 doesnt mean ascii 0. '0' means ascii 0.  See the difference?  eg.


#include <iostream>
int main() {
  if (0 == '\0') {
    std::cout << "I am seriously confused\n";
  }
  return 0;
}

[jun_fang, 2014-07-30 17:50:33]

On 2014/07/30 17:13:50, Tom Sepez wrote:
> Are you sure? 0 has the ascii value 48 but '\0' has the ascii value 0. They
are
> > different.
> 
> 0 doesnt mean ascii 0. '0' means ascii 0.  See the difference?  eg.
> 
> 
> #include <iostream>
> int main() {
>   if (0 == '\0') {
>     std::cout << "I am seriously confused\n";
>   }
>   return 0;
> }


Yes. You are right.

[jun_fang, 2014-07-30 17:51:11]

On 2014/07/30 17:50:33, jun_fang wrote:
> On 2014/07/30 17:13:50, Tom Sepez wrote:
> > Are you sure? 0 has the ascii value 48 but '\0' has the ascii value 0. They
> are
> > > different.
> > 
> > 0 doesnt mean ascii 0. '0' means ascii 0.  See the difference?  eg.
> > 
> > 
> > #include <iostream>
> > int main() {
> >   if (0 == '\0') {
> >     std::cout << "I am seriously confused\n";
> >   }
> >   return 0;
> > }
> 
> 
> Yes. You are right.

LGTM

[Tom Sepez, 2014-07-30 20:04:08]

Committed patchset #4 manually as r0d3b5cc (presubmit successful).