Bug 45391

You forgot to attach something!

Created attachment 73979 [details] Test case Sorry for the late reply, the file should be attached now.

Created attachment 74996 [details] Patch Make sure that the length passed to fromRawDataWithoutRef() does not exceed the length of the string. This is essentially what happens in FontFastPath.cpp since that one uses WidthIterator for getting the 'to' value.

Comment on attachment 74996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74996&action=review > WebKit/qt/tests/qwebview/tst_qwebview.cpp:325 > + QTest::qWait(1000); Why this long wait?

(In reply to comment #4) > (From update of attachment 74996 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74996&action=review > > > WebKit/qt/tests/qwebview/tst_qwebview.cpp:325 > > + QTest::qWait(1000); > > Why this long wait? To make sure the next paint event is received. Is there a way in QTestLib to wait for a particular event perhaps?

(In reply to comment #5) > (In reply to comment #4) > > Why this long wait? > > To make sure the next paint event is received. Is there a way in QTestLib to wait for a particular event perhaps? Looks like a simple QApplication::processEvent() is sufficient to trigger the problem. I'll upload a new patch.

Created attachment 74997 [details] Patch

It is tricky to test this reliably, processEvents() may be enough on one machine, it may not on another one. On desktop machines it's fairly easy to get somewhat reliable results, but on slower machines with entirely different windowing systems it becomes difficult. (something we've observed when running on meego/maemo devices for example) Would this assertion trigger anyway when running the tests on the bots? After all we're doing release builds there. I'm not a big fan of adding these kind of tests, given that there's a very small chance of actually catching a regression with them. At the same time they cause headaches when running the tests on other platforms. I like the qWait() approach over calling processEvents() FWIW.

Comment on attachment 74997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74997&action=review > WebKit/qt/tests/qwebview/tst_qwebview.cpp:325 > + QApplication::processEvents(); Hmm, in this particular case I wonder if this is going to work at all on platforms that are slightly more asynchronous, such as Mac OS X?

Comment on attachment 74997 [details] Patch r- due to Simon's comments.

(In reply to comment #9) > (From update of attachment 74997 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74997&action=review > > > WebKit/qt/tests/qwebview/tst_qwebview.cpp:325 > > + QApplication::processEvents(); > > Hmm, in this particular case I wonder if this is going to work at all on platforms that are slightly more asynchronous, such as Mac OS X? This was tested on OS X actually. I can go back to qWait() if that's preferrable, or even use a QEventLoop to verify that a QPaintEvent has been received by the view.

Created attachment 75002 [details] Patch Updated the test to explicitly wait (up to 5 seconds) for the next QPaintEvent after updating the selection

(In reply to comment #8) > Would this assertion trigger anyway when running the tests on the bots? After all we're doing release builds there. Not entirely sure, but the QCOMPARE() at the end would fail even if the test does not assert in release mode.

The commit-queue encountered the following flaky tests while processing attachment 74997 [details]: http/tests/security/xssAuditor/dom-write-innerHTML.html fast/profiler/throw-exception-from-eval.html Please file bugs against the tests. These tests were authored by abarth@webkit.org, dbates@webkit.org, kmccullough@apple.com, oliver@apple.com, and timothy@apple.com. The commit-queue is continuing to process your patch.

(In reply to comment #13) > (In reply to comment #8) > > Would this assertion trigger anyway when running the tests on the bots? After all we're doing release builds there. > > Not entirely sure, but the QCOMPARE() at the end would fail even if the test does not assert in release mode. Actually, that's not true, the test will pass with the assertion not active. What I'd want to verify is the selection rectangle, but I can't find any way of retrieving that. Any suggestions?

Created attachment 75008 [details] Patch New patch without a test

Comment on attachment 75008 [details] Patch Clearing flags on attachment: 75008 Committed r72788: <http://trac.webkit.org/changeset/72788>

All reviewed patches have been landed. Closing bug.

Shouldn't this be cherry picked into qtwebkit-2.1 ? There is a downstream report of a crash that seems to have been caused by this bug. See https://bugs.kde.org/show_bug.cgi?id=259272.

(In reply to comment #19) > Shouldn't this be cherry picked into qtwebkit-2.1 ? There is a downstream report of a crash that seems to have been caused by this bug. See https://bugs.kde.org/show_bug.cgi?id=259272. I would not cherry pick targeting 2.1 for an assert. It is late in the process, only important changes should go in. Qt scripts to build webkit always build in release.

(In reply to comment #20) > (In reply to comment #19) > > Shouldn't this be cherry picked into qtwebkit-2.1 ? There is a downstream report of a crash that seems to have been caused by this bug. See https://bugs.kde.org/show_bug.cgi?id=259272. > > I would not cherry pick targeting 2.1 for an assert. It is late in the process, only important changes should go in. > Qt scripts to build webkit always build in release. I believe a fix for a crash that happens by openging a web page is worth including in the release (or in a later minor update)... It can even be classified as a security vuln (at minimum it's a DoS). The problem I have is that I can't reproduce the crash with the supplied example: Inside Font::selectionRectForText(), codePath(run) returns Complex and thus Font::selectionRectForSimpleText() (where the problem is) never gets called. Anybody familiar with the code could tell what should I do to actually call Font::selectionRectForSimpleText()?

(In reply to comment #21) > I believe a fix for a crash that happens by openging a web page is worth including in the release (or in a later minor update)... It can even be classified as a security vuln (at minimum it's a DoS). Yep, I was under the assumption it is only invalid selection and assertion in debug. If this can cause a crash in release, I agree this need to go in 2.0 and 2.1.

(In reply to comment #22) > (In reply to comment #21) > > I believe a fix for a crash that happens by openging a web page is worth including in the release (or in a later minor update)... It can even be classified as a security vuln (at minimum it's a DoS). > > Yep, I was under the assumption it is only invalid selection and assertion in debug. If this can cause a crash in release, I agree this need to go in 2.0 and 2.1. I didn't see any crashes when testing in release mode myself but using a QString returned by fromRawDataWithoutRef() could certainly read and write to memory out of bounds.

(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > I believe a fix for a crash that happens by openging a web page is worth including in the release (or in a later minor update)... It can even be classified as a security vuln (at minimum it's a DoS). > > > > Yep, I was under the assumption it is only invalid selection and assertion in debug. If this can cause a crash in release, I agree this need to go in 2.0 and 2.1. > > I didn't see any crashes when testing in release mode myself but using a QString returned by fromRawDataWithoutRef() could certainly read and write to memory out of bounds. So it's a simple fix and has potential security implications. I'm adding it as a blocker for 2.1 and 2.0.

Revision r72788 cherry-picked into qtwebkit-2.1 with commit 6aef00a <http://gitorious.org/webkit/qtwebkit/commit/6aef00a>