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

[Andrew Azores]

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,

-- 
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/20131009/ff9942e9/PR539-append-null.patch