Page MenuHomePhabricator

Using unchecked input as array keys may cause TypeErrors
Closed, ResolvedPublic

Description

Author: mr.heat

Description:
As suggested in bug 54646 I report this as a separate issue. Unfortunately all I have is this screenshot:

https://skydrive.live.com/?cid=89518794CBF0E4D2&id=89518794CBF0E4D2!520

There is not much to see in the screenshot, basically three lines in the Firefox web console:

ULS: Unknown language doi. (load.php:236)
ULS: Unknown language fonipa. (load.php:236)
TypeError: $.ime.sources[name] is undefined (load.php:108)

It seems this error stopped execution of all following scripts including my [[dewiki:User:TMg/autoFormatter]]. It seems this happened in the moment my script tried to focus the edit window. Some ULS code is loaded and executed in this moment. From the users point of view my script stopped working.

I spend some hours debugging and checking all scripts involved (starting with mine, of course) but wasn't able to reproduce this issue. The screenshot suggests it's something in the ULS code.

While debugging I saw code like this:

$.ime.languages[<key>].<property>
$.ime.sources[<key>].<property>

This code may possibly fail. If the <key> comes from a source that is somewhere broken (e.g. from an old ULS version) there may be no array element with the given key. Accessing a <property> of an undefined array element makes the code crash. My suggestion is to add a few simple checks to some of these lines.

One of these lines is in the prepareInputMethods function which may be the line "108" from the screenshot:

https://github.com/wikimedia/jquery.ime/blob/master/src/jquery.ime.selector.js

Relevant source:

var language = $.ime.languages[languageCode];
$.each( language.inputmethods, function ( index, inputmethod ) {

var name = $.ime.sources[inputmethod].name;

}

The problem is that the keys are read from the "languages" array but are used to access the "sources" array. If these two arrays are out of sync (for whatever reason) the code crashs.

Suggestion:

var language = $.ime.languages[languageCode];
$.each( language.inputmethods, function ( index, inputmethod ) {

var source = $.ime.sources[inputmethod];
if ( !source ) {
  continue;
}
var name = source.name;

}

Since I'm not really sure if this is the only line that may cause the described problem the same check should be added to a few more lines, if appropriate.


Version: unspecified
Severity: normal

Details

Reference
bz55701

Event Timeline

bzimport raised the priority of this task from to High.
bzimport set Reference to bz55701.
bzimport created this task.Oct 14 2013, 4:01 PM

https://github.com/wikimedia/jquery.ime/commit/ea9ee736 must be the cause. The id of the input method was changed. That itself is harmless. But when the old source is served from cache, it can create problems. Eventhough it is very unlikely that new problems arise because of that old change, a validation as suggested above is nice.

https://gerrit.wikimedia.org/r/#/c/97068 ported this to MediaWiki. Marking fixed.

Change 120212 had a related patch set uploaded by Thiemo Mättig (WMDE):
Make sure script execution doesn't stop by assuming unchecked input is set

https://gerrit.wikimedia.org/r/120212

Change 120212 merged by jenkins-bot:
Make sure script execution doesn't stop by assuming unchecked input is set

https://gerrit.wikimedia.org/r/120212