Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
423953002:
Tidy up app::response(). (Closed)
DescriptionTidy up app::response().
Follow-up from https://codereview.chromium.org/424883002/
- Remove some stray whitespace.
- Fix "else after return".
- Remove unused swResponse local.
- Treat unexpectedly large responses as errors.
BUG=
R=jun_fang@foxitsoftware.com
Committed: https://pdfium.googlesource.com/pdfium/+/621d4de
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments. #
Total comments: 2
Patch Set 3 : Tolerate out-of-range returns. #Patch Set 4 : Tolerate out-of-range returns #
Total comments: 1
Patch Set 5 : Search for U+0000. #Patch Set 6 : Search for U+0000. #
Total comments: 2
Patch Set 7 : Use returned length, update API comment. #Patch Set 8 : Tabs. #Patch Set 9 : Wordsmith #Messages
Total messages: 28 (0 generated)
Jun, please review. Thanks.
https://codereview.chromium.org/423953002/diff/1/fpdfsdk/src/javascript/app.cpp File fpdfsdk/src/javascript/app.cpp (right): https://codereview.chromium.org/423953002/diff/1/fpdfsdk/src/javascript/app.c... fpdfsdk/src/javascript/app.cpp:1103: char* pBuff = new char[nLength]; nLength is the length of content. Doesn't include the last two chars '\0' in the following wide string. So allocate 2 more chars here. char* pBuff = new char[nLength+2]; Initialize all bytes with '\0' to indicate the ending char when the buffer is saved with contents. memset(pBuff, 0, nLength+2); https://codereview.chromium.org/423953002/diff/1/fpdfsdk/src/javascript/app.c... fpdfsdk/src/javascript/app.cpp:1105: if (nLength <= 0 || nLength > sizeof(pBuff)) Sizeof(pBuff) is 4 which represents bytes of a pointer. It may not work correctly here. You may use strlen(pBuff).
Thanks. I'll take care of this. https://codereview.chromium.org/423953002/diff/1/fpdfsdk/src/javascript/app.cpp File fpdfsdk/src/javascript/app.cpp (right): https://codereview.chromium.org/423953002/diff/1/fpdfsdk/src/javascript/app.c... fpdfsdk/src/javascript/app.cpp:1103: char* pBuff = new char[nLength]; On 2014/07/29 04:11:30, jun_fang wrote: > nLength is the length of content. Doesn't include the last two chars '\0' in the > following wide string. So allocate 2 more chars here. > char* pBuff = new char[nLength+2]; > Yeah, I'm having a hard time reconciling this with the description of the API in fpdfformfill.h that says "Number of bytes the user input text consumes, not including trailing zeros. If the text exceed 2048 bytes, the exceeded part will be ignored. Does this mean that there are trailing zeros written to the buffer by the method and they're just not counted? Or can we not expect any to be written unless we put them there in the caller? If the input is 2048 bytes, will we get 2048 bytes plus an out-of-bounds trailing (0 0)? Or will we get 2048 bytes and no zeros? Or will we get 2046 bytes and trailing (0 0). > Initialize all bytes with '\0' to indicate the ending char when the buffer is > saved with contents. > memset(pBuff, 0, nLength+2); https://codereview.chromium.org/423953002/diff/1/fpdfsdk/src/javascript/app.c... fpdfsdk/src/javascript/app.cpp:1105: if (nLength <= 0 || nLength > sizeof(pBuff)) On 2014/07/29 04:11:30, jun_fang wrote: > Sizeof(pBuff) is 4 which represents bytes of a pointer. It may not work > correctly here. You may use strlen(pBuff). Good catch. Looks like this has been wrong for years! Strlen might be bad depending upon the way we handle nulls above (we shouldn't really need them if we have explicit lengths, right?). And walking a string to find its length when we were just told it by the method seems wasteful.
https://codereview.chromium.org/423953002/diff/1/fpdfsdk/src/javascript/app.cpp File fpdfsdk/src/javascript/app.cpp (right): https://codereview.chromium.org/423953002/diff/1/fpdfsdk/src/javascript/app.c... fpdfsdk/src/javascript/app.cpp:1103: char* pBuff = new char[nLength]; On 2014/07/29 16:55:57, Tom Sepez wrote: > On 2014/07/29 04:11:30, jun_fang wrote: > > nLength is the length of content. Doesn't include the last two chars '\0' in > the > > following wide string. So allocate 2 more chars here. > > char* pBuff = new char[nLength+2]; > > > Yeah, I'm having a hard time reconciling this with the description of the API in > fpdfformfill.h that says "Number of bytes the user input text consumes, not > including trailing zeros. If the text exceed 2048 bytes, the exceeded part will > be ignored. > > Does this mean that there are trailing zeros written to the buffer by the method > and they're just not counted? Or can we not expect any to be written unless we > put them there in the caller? If the input is 2048 bytes, will we get 2048 > bytes plus an out-of-bounds trailing (0 0)? Or will we get 2048 bytes and no > zeros? Or will we get 2046 bytes and trailing (0 0). > > > Initialize all bytes with '\0' to indicate the ending char when the buffer is > > saved with contents. > > memset(pBuff, 0, nLength+2); > In theory, there is no difference between 2046 plus trailing (0 0) and 2048 plus trailing (0 0). If we use 2046 plus trailing (0 0), all length used as parameters should be changed to length-2, it's not as popular as 2048 plus trailing (0 0). People always think that length represents the length of data. In the latter way (2048 plus trailing(0 0), we just allocate 2 more bytes and keep the parameter of length unchanged. https://codereview.chromium.org/423953002/diff/1/fpdfsdk/src/javascript/app.c... fpdfsdk/src/javascript/app.cpp:1105: if (nLength <= 0 || nLength > sizeof(pBuff)) On 2014/07/29 16:55:57, Tom Sepez wrote: > On 2014/07/29 04:11:30, jun_fang wrote: > > Sizeof(pBuff) is 4 which represents bytes of a pointer. It may not work > > correctly here. You may use strlen(pBuff). > Good catch. Looks like this has been wrong for years! Strlen might be bad > depending upon the way we handle nulls above (we shouldn't really need them if > we have explicit lengths, right?). And walking a string to find its length when > we were just told it by the method seems wasteful. Strlen depends upon the last char '\0'. That's why I suggest to initialize the buffer with '\0' first. Even walking a string, we don't have a rule to judge which char is the last one.
> Strlen depends upon the last char '\0'. That's why I suggest to initialize the > buffer with '\0' first. Even walking a string, we don't have a rule to judge > which char is the last one. But ... strlen() won't work since this is utf16. And if we're using explicit lengths, we probably don't need a trailing 0. But as a defensive measure, I'll keep it.
Jun, please take a look at PS#2. Thanks.
On 2014/07/29 17:39:05, Tom Sepez wrote: > > Strlen depends upon the last char '\0'. That's why I suggest to initialize the > > buffer with '\0' first. Even walking a string, we don't have a rule to judge > > which char is the last one. > But ... strlen() won't work since this is utf16. And if we're using explicit > lengths, we probably don't need a trailing 0. But as a defensive measure, I'll > keep it. For this case, strlen is only used before this char string is converted to utf16. It should be fine to use strlen. For original code, I guess the programmer thought if there was a conflicting between returned length and the actual length of data (getting from sizeof() or strlen, we should use the actual length of data rather than the returned length.
https://codereview.chromium.org/423953002/diff/20001/fpdfsdk/src/javascript/a... File fpdfsdk/src/javascript/app.cpp (right): https://codereview.chromium.org/423953002/diff/20001/fpdfsdk/src/javascript/a... fpdfsdk/src/javascript/app.cpp:1110: if (nLengthBytes <= 0 || nLengthBytes > kMaxInputBytes) I saw the comments for the function of app_response. int (*app_response)(struct _IPDF_JsPlatform* pThis, FPDF_WIDESTRING Question, FPDF_WIDESTRING Title, FPDF_WIDESTRING Default, FPDF_WIDESTRING cLabel, FPDF_BOOL bPassword, void* response, int length); * Return Value: * Number of bytes the user input text consumes, not including trailing zeros. If the text exceed 2048 bytes, the exceeded part will be ignored. According to the comments, we change 2048 bytes to UTF16 rather than returning a false.
> we should use the actual length of data rather than the returned length. No, I think you're confused. The documentation says "No matter on what platform, the response should be always input in UTF-16LE encoding." which I interpret to mean "No matter on what platform, the |response| buffer should be always be written to by the method in UTF-16LE encoding", since response is not used on input (e.g. there is a separate parameters for default). We then call CFX_WideString::FromUTF16LE(), not CFX_WideString::FromLocal(), and FromUTF16LE honor's its length argument without searching for 0s. I guess what you're saying is we shouldn't trust the method to return an actual length. But that seems overly paranoid to me.
https://codereview.chromium.org/423953002/diff/20001/fpdfsdk/src/javascript/a... File fpdfsdk/src/javascript/app.cpp (right): https://codereview.chromium.org/423953002/diff/20001/fpdfsdk/src/javascript/a... fpdfsdk/src/javascript/app.cpp:1103: const int kMaxInputBytes = 2048; It's suggested to use uppercase like MAX_INPUT_BYPTES to define a const variable.
> * Return Value: > * Number of bytes the user input text consumes, not including trailing zeros. If > the text exceed 2048 bytes, the exceeded part will be ignored. I interpreted this to mean "If the user enters more that 2048 bytes, we truncate the response buffer at 2048 and return a value of 2048." But I could imagine someone interpreting this as a diretion to returning more than 2048 -- the full length". So I'll fix that.
If > > the text exceed 2048 bytes, the exceeded part will be ignored. > I interpreted this to mean "If the user enters more that 2048 bytes, we truncate > the response buffer at 2048 and return a value of 2048." But I could imagine > someone interpreting this as a diretion to returning more than 2048 -- the full > length". So I'll fix that. Or more precisely, "If the user enters more that 2048 bytes, the method must trunacte the response buffer at 2048 and return a value of 2048".
Ok, I think we're on the same page now. Please take a look at PS#4, and thanks for your help.
On 2014/07/29 18:21:15, Tom Sepez wrote: > Ok, I think we're on the same page now. Please take a look at PS#4, and thanks > for your help. One other thing I don't understand is that the app_response() method has this same boilerplate as the other methods, which I don't think applies. 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. In this case, the buffer will not be modified. Which contradicts the "truncation interpretation" above, e.g. if we get back 2052 as the result, we cant' trust the buffer.
https://codereview.chromium.org/423953002/diff/60001/fpdfsdk/src/javascript/a... File fpdfsdk/src/javascript/app.cpp (right): https://codereview.chromium.org/423953002/diff/60001/fpdfsdk/src/javascript/a... fpdfsdk/src/javascript/app.cpp:1106: return FALSE; very minor issue here. It's not aligned with other code in the same level. :-)
On 2014/07/29 18:31:31, jun_fang wrote: > https://codereview.chromium.org/423953002/diff/60001/fpdfsdk/src/javascript/a... > File fpdfsdk/src/javascript/app.cpp (right): > > https://codereview.chromium.org/423953002/diff/60001/fpdfsdk/src/javascript/a... > fpdfsdk/src/javascript/app.cpp:1106: return FALSE; > very minor issue here. It's not aligned with other code in the same level. :-) Ok, done, tabs are always tricky. I went back to the approach you were asking for in the earlier comments, since I think that the API description is confusing enough that trusting the method return is risky. Please re-review.
https://codereview.chromium.org/423953002/diff/90001/fpdfsdk/src/javascript/a... File fpdfsdk/src/javascript/app.cpp (right): https://codereview.chromium.org/423953002/diff/90001/fpdfsdk/src/javascript/a... fpdfsdk/src/javascript/app.cpp:1111: if (nLengthBytes <= 0) hmm. Just noticed this. Why do we think that an empty string isn't an OK response?
On 2014/07/29 18:27:46, Tom Sepez wrote: > On 2014/07/29 18:21:15, Tom Sepez wrote: > > Ok, I think we're on the same page now. Please take a look at PS#4, and > thanks > > for your help. > One other thing I don't understand is that the app_response() method has this > same boilerplate as the > other methods, which I don't think applies. > > 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. In > this case, the buffer will not be modified. > > Which contradicts the "truncation interpretation" above, e.g. if we get back > 2052 as the result, we cant' trust the buffer. Please see the comments about the method of app_response as below: * Method: app_response * Displays a dialog box containing a question and an entry field for the user to reply to the question. int (*app_response)(struct _IPDF_JsPlatform* pThis, FPDF_WIDESTRING Question, FPDF_WIDESTRING Title, FPDF_WIDESTRING Default, FPDF_WIDESTRING cLabel, FPDF_BOOL bPassword, void* response, int length); * Comments: * No matter on what platform, the response should be always input in UTF-16LE encoding. * 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. In this case, the buffer will not * be modified. 'response' is pointed to the buffer that receives the user's answer.According to the comments as above, the return value can be larger than 2048.
https://codereview.chromium.org/423953002/diff/90001/fpdfsdk/src/javascript/a... File fpdfsdk/src/javascript/app.cpp (right): https://codereview.chromium.org/423953002/diff/90001/fpdfsdk/src/javascript/a... fpdfsdk/src/javascript/app.cpp:1111: if (nLengthBytes <= 0) On 2014/07/29 18:53:37, Tom Sepez wrote: > hmm. Just noticed this. Why do we think that an empty string isn't an OK > response? An empty string means that users don't input anythings.So we don't handle it.
> 'response' is pointed to the buffer that receives the user's answer.According to > the comments as above, the return value can be larger than 2048. Yep. But I'm having a hard time reconciling "If the text exceed 2048 bytes, the exceeded part will be ignored." with "The return value always indicated number of bytes required for the buffer, even when ... In this case, the buffer will not be modified." So which is it? If the user types 2052 bytes, it sure sounds like we return 2052, but does |response| contain 2048 bytes or is it unmodified?
On 2014/07/29 19:04:19, Tom Sepez wrote: > > 'response' is pointed to the buffer that receives the user's answer.According > to > > the comments as above, the return value can be larger than 2048. > > Yep. But I'm having a hard time reconciling > "If the text exceed 2048 bytes, the exceeded part will be ignored." with > "The return value always indicated number of bytes required for the buffer, even > when ... In this case, > the buffer will not be modified." > > So which is it? If the user types 2052 bytes, it sure sounds like we return > 2052, but does |response| contain 2048 bytes or is it unmodified? If the user types 2052 bytes, |response| will keep 2048 bytes. The app_response returns 2052 to the caller to indicate the messages that the user types need 2052 types. This information is useful for some callers although it is't handled specially in the current code. Some callers can increase the size of buffer to receive messages from users next time.
> If the user types 2052 bytes, |response| will keep 2048 bytes. The app_response > returns 2052 to the caller to indicate the messages that the user types need > 2052 types. This information is useful for some callers although it is't handled > specially in the current code. Some callers can increase the size of buffer to > receive messages from users next time. Ok. So the comment "In this case, the buffer will not be modified." is wrong and should be removed?
On 2014/07/29 19:17:02, Tom Sepez wrote: > > If the user types 2052 bytes, |response| will keep 2048 bytes. The > app_response > > returns 2052 to the caller to indicate the messages that the user types need > > 2052 types. This information is useful for some callers although it is't > handled > > specially in the current code. Some callers can increase the size of buffer to > > receive messages from users next time. > > Ok. So the comment "In this case, the buffer will not be modified." is wrong > and should be removed? I agree to remove it.
> I agree to remove it. Excellent. So which do you prefer, PS#6 or PS#4?
On 2014/07/29 19:32:33, Tom Sepez wrote: > > I agree to remove it. > Excellent. So which do you prefer, PS#6 or PS#4? I prefer PS#4.
Ok, please review.
On 2014/07/29 20:25:53, Tom Sepez wrote: > Ok, please review. Thanks. LGTM
Message was sent while issue was closed.
Committed patchset #9 manually as r621d4de (presubmit successful). |
