Bug 40447 – Infinite loop in WebInspector.TextEditorModel.prototype._replaceTabsIfNeeded

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Gustaaf Groenendaal (MysteryQuest)]

Original bug here: https://bugs.webkit.org/show_bug.cgi?id=40304
Opening the page provided in the original bug: ( http://j.benzakoun.free.fr/JSNE/JavaScriptNoteEditor_b2/index.php?loadid=13&key=d16ad39b783f65e55410ab5d8ca9b4ab )
Seems to start a buffer overflow in: Web Inspector -> recources -> <file> -> contents
Can confirm that this is happening on all Macs with 10.5 and up where Safari 5 or Nightly installed.
Needs a check on other platforms to see if this is Mac only or not.

Credit to the original reporter.
Passing this on as a security bug as I don't know the risk of the cause of this bug being available in public.

[David Kilzer (:ddkilzer)]

This appears to be a denial-of-service (DoS) attack, or at least a performance issue.  I see a lot of RAM being allocated when tapping on the "Content" tab of "index.php" in the Web Inspector, but nothing else.

I think this should be duped back to Bug 40304 unless there is more information available.  (Can you attach a crash log, Gustaaf?)

[Pavel Feldman]

It is just an infinite loop in WebInspector.TextEditorModel.prototype._replaceTabsIfNeeded. I'll fix it.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> It is just an infinite loop in WebInspector.TextEditorModel.prototype._replaceTabsIfNeeded. I'll fix it.

Does this really need to be in Security if it's an infinite loop?

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Does this really need to be in Security if it's an infinite loop?

Removing from Security group.

Created attachment 98905 [details]
Fix performance / memory allocation problem

[Pavel Feldman]

Comment on attachment 98905 [details]
Fix performance / memory allocation problem

View in context: https://bugs.webkit.org/attachment.cgi?id=98905&action=review

Looks good. Please fix the log entry at least.

> Source/WebCore/ChangeLog:5
> +        Infinite loop in WebInspector.TextEditorModel.prototype._replaceTabsIfNeeded

You should change the log entry in case it was not in fact an infinite loop. Also, please start bug title with the "Web Inspector:".

> Source/WebCore/inspector/front-end/TextEditorModel.js:152
> +            var sum = 0;

I'd rename "sum" to the "offset".

> Source/WebCore/inspector/front-end/TextEditorModel.js:164
> +            if (line.length > caretIndex) {

no need for { }

> LayoutTests/inspector/editor/text-editor-model-replace-tabs.html:15
> +        InspectorTest.addResult("Output length:" + lines[0].length + ", output matches sample:" + (lines[0] === sampleOutput));

We would typically dump "text" and "sampleOutput" instead. It is way more informative.

Created attachment 99255 [details]
Fix performance / memory allocation problem

[Pavel Feldman]

Comment on attachment 99255 [details]
Fix performance / memory allocation problem

View in context: https://bugs.webkit.org/attachment.cgi?id=99255&action=review

> Source/WebCore/ChangeLog:5
> +        Performance / memory allocation issue in WebInspector.TextEditorModel.prototype._replaceTabsIfNeeded

Web Inspector: prefix missing

> LayoutTests/ChangeLog:5
> +        Performance / memory allocation issue in WebInspector.TextEditorModel.prototype._replaceTabsIfNeeded

ditto

> LayoutTests/inspector/editor/text-editor-model-replace-tabs-expected.txt:16
> +[]

It'd be nicer to print results side by side with the source lines. You should also render tab as \t in the source to avoid possible confusions (I have tab rendered as 8 spaces in my settings).

> LayoutTests/inspector/editor/text-editor-model-replace-tabs.html:35
> +            result = "FAIL: time limit (100ms) exceeded (execution time: " + duration + "ms)"

This won't work on the bots. Things may take seconds there. Please do not test performance in the layout tests - they become flaky. As we agree, measuring correctness is sufficient.

Created attachment 99278 [details]
Fix performance / memory allocation problem

[Pavel Feldman]

Comment on attachment 99278 [details]
Fix performance / memory allocation problem

View in context: https://bugs.webkit.org/attachment.cgi?id=99278&action=review

> Source/WebCore/ChangeLog:5
> +        Performance / memory allocation issue in WebInspector.TextEditorModel.prototype._replaceTabsIfNeeded

Please add WebInspector: prefix.

> LayoutTests/ChangeLog:5
> +        Performance / memory allocation issue in WebInspector.TextEditorModel.prototype._replaceTabsIfNeeded

Ditto

> LayoutTests/inspector/editor/text-editor-model-replace-tabs.html:12
> +    	var input = testCases[i];

weird indent, otherwise looks good.

> LayoutTests/inspector/editor/text-editor-model-replace-tabs.html:22
> +    var bufT = [];

nit: no abbreviations in webkit (bufferT & bufferS)

Created attachment 99303 [details]
WebInspector: Fix performance / memory allocation problem

[WebKit Review Bot]

Comment on attachment 99303 [details]
WebInspector: Fix performance / memory allocation problem

Clearing flags on attachment: 99303

Committed r90209: <http://trac.webkit.org/changeset/90209>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.