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

[Jiri Vanek]

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.