509853 - Infinite loop in Lookup parser causes memory leak and freezes the system

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.3pre) Gecko/20090810 Shiretoko/3.5.3pre ID:20090810040503 and Mozmill 1.2

While creating the helper function for notification bars in bug 509031 I have noticed that when running a test which uses the new function let the complete system freeze. Within 1s Firefox allocates around 1.7GB of my memory and the system starts to swap out unnecessary memory. You have nearly no control anymore unless you immediately kill the Firefox process. 

The following function demonstrates this problem:

var testRecorded = function () {
  controller = mozmill.getBrowserController();
  controller.click(new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser")/id("appcontent")/id("content")/anon({"anonid":"tabbox"})/anon({"anonid":"panelcontainer"})/{"id":"panel1250031197521"}"/{"value":"popup-blocked"}/anon({"type":"warning"})/{"class":"messageCloseButton tabbable"}'));
}

It's not only for click but also for any other function which accesses the new created element.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: testcase

Pasting the test directly breaks the line feeds. Lets add it as attachment and raise the freeze by just accessing getNode().

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Stack

I used gdb to get a stack while the nodeSearch function is executed. The JS stack looks really weird.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given the output of the JS stack we have an infinite loop here:

var smartSplit = function (str) {
  // Note: I would love it if someone good with regular expressions 
  // could just replace this function with a good regex
  var repls = [];
  while (str.indexOf('"') != -1) {
    var i = str.indexOf('"');
    var s = str.slice(i, str.indexOf('"', i + 1) +1)
    var str = str.replace(s, '%$^'+repls.length);
    repls.push(s)
  }

Comment 4

[cmtalbert]

Ok, I've debugged this.  Here is the expression again that is causing the issue:
var elem = new elementslib.Lookup(controller.window.document,
  '/id("main-window")/id("browser")/id("appcontent")/id("content")
   /anon({"anonid":"tabbox"})/anon({"anonid":"panelcontainer"})
   /{"id":"panel1250031197521"}"/{"value":"popup-blocked"}
   /anon({"type":"warning"})/{"class":"messageCloseButton tabbable"}');

Do you see that extra quote mark after "panel1250031197521"}?  That's what does it.  The parser goes all kinds of screwy and starts reading in data from way outside the string, putting random memory into repls.  The memory leak occurs because we keep pushing these data items onto the array.

The fix here is two fold:
1. Correct the generation mechanism in the inspector not to output invalid lookup expressions -- Henrik --> Did you edit this expression and put that extra quote in there by accident?  Or was it generated?  Can you check?
2. Fix this loop so that when things like this go wrong we don't just loop infinitely.  There are two fixes here, the easy one is to gate the loop conditional with the total length of the string.  If i is beyond the length of the string, we should quit.  And if there is an odd number of quote marks we should just throw and refuse to parse.

Patch coming.

Comment 5

[cmtalbert]

Attachment: Patch to fix issues in lookup code

This is a patch that fixes the issues in the lookup parser.
It makes sure we don't have unmatched quotes and provides a secondary check on the while loop to be certain that it doesn't become infinite.

I'm sort of assuming the extra " got in the lookup expression by accidental editing.  Henrik, if the inspector generated that invalid lookup expression, let me know so I can fix that and add it to this patch.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No, it was my fault. The search & replace action was wrong. Even after I checked the lookup string multiple times I haven't seen it. That's most of the fact that the first ones I replaced manually. I'll update my patch on bug 509031.

Thanks for the patch Clint.

Comment 7

[cmtalbert]

Comment on attachment 394602 [details] [diff] [review]
Patch to fix issues in lookup code

r+'d on IRC.

Comment 8

[cmtalbert]

rev 544 --> Fixed

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks good. Thanks Clint! Marking verified.