[rfc][icedtea-web] Malformed messages from IcedTeaPluginRequestProcessor (PR539)

[Andrew Azores]

On 10/10/2013 04:48 AM, Jiri Vanek wrote:
> On 10/09/2013 08:02 PM, Andrew Azores wrote:
>> Hi,
>>
>> PR539 was reopened with a comment that the applet at [1] no longer 
>> works. Looking through the
>> provided logs, an index out of bounds exception is occurring at line 
>> 53 of
>> GetMemberPluginCallRequest.java. This happens because the parseReturn 
>> method expects its String
>> param to be of the form "reference 28 JavaScriptToString 90", but 
>> occasionally it can occur that the
>> param is instead of the form "reference 30 JavaScriptToString ", 
>> missing the second numeric value.
>> This happens because of _getString and _getMember in 
>> IcedTeaPluginRequestProcessor.cc. If
>> data->call_successful is false in either of these functions, then 
>> data->result is never modified
>> from the value it held before data was passed into the functions, 
>> which means it is an empty string.
>> The data->call_successful struct member is set by a call to 
>> NPNetscapeFuncs so I'm not entirely sure
>> why it is coming back false in these cases.
>>
>> It's certainly not an expected situation for these messages to be 
>> malformed, which is especially
>> evident looking around the codebase. Many other places do similar 
>> work as done in
>> GetMemberPluginCallRequest.parseReturn(String), where message.split(" 
>> ") is called and the length of
>> the resulting array never validated. Adding a length validation to 
>> parseReturn seems to fix this
>> particular bug (the applet loads at least, but I'm not sure what else 
>> this bug may be affecting) but
>> doesn't address the issue of the message being malformed in the first 
>> place. It seems like the
>> expected behaviour at some point in the past would be for these 
>> messages instead to have "null" or
>> "void":
>>
>>          if (!"null".equals(args[3]) && !"void".equals(args[3]))
>>
>> appended to them rather than the empty string, but that does not seem 
>> to be the case now. Appending
>> this to the data->result string is easy enough, it can just be done 
>> in a new else-clause for
>> data->call_successful in _getString and _getMember. As far as I'm 
>> able to tell this would be the
>> earliest feasible place to do the addition. It could also be done in
>> IcedTeaPluginRequestProcessor::sendString or ::sendMember, but these 
>> call _getString and _getMember
>> respectively through IcedTeaPluginUtilities::callAndWaitForResult 
>> anyway. The patch takes the
>> approach of adding it to _getString and _getMember. Other functions 
>> in the file, such as _eval and
>> _call, append "0" when call_successful is false, so this approach 
>> makes the most sense to me. I
>> don't see any problems that this might cause and running the 
>> reproducers showed no new failures either.
>>
>> ChangeLog:
>> Fix array index out of bounds due to malformed plugin message (PR539)
>> * plugin/icedteanp/IcedTeaPluginRequestProcessor.cc: (_getMember, 
>> _getString) append "null" to
>> result when call is unsuccessful
>>
>> [1] 
>> https://www.bankid.no/Hjelp-og-nyttige-verktoy/Nyttige-verktoy/Test-din-BankID/
>>
>> Thanks,
>>
>
>
> What about any test on this? It woould deserve at least C++ test, or 
> mock test or reproducer (in best case all three :)
>
> Otherwise give sense to me.
>
>
> J.

Took me a while to figure out how to make a JavaScript object that 
doesn't respond to toString(), but I got it. Attached both patches again.

Note that this reproducer will currently fail whether or not you apply 
the fix patch. Note that commit 420d72e5cee7, the one I proposed be 
backed out a while ago, is a blocker on this bug as well. If you "hg 
backout -r 420d72e5cee7" and then apply the reproducer patch, it will 
fail in the manner I intend it to. Then when you apply the fix, the 
reproducer will pass.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR539-append-null.patch
Type: text/x-patch
Size: 892 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131010/34e9a2b7/PR539-append-null.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR539-reproducer.patch
Type: text/x-patch
Size: 5437 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131010/34e9a2b7/PR539-reproducer.patch