Last Comment Bug 264956 - Malformed HTML document causes crash - M173 FF10PR1 [@ RuleProcessorData::RuleProcessorData]
: Malformed HTML document causes crash - M173 FF10PR1 [@ RuleProcessorData::Rul...
Status: VERIFIED FIXED
[sg:dos]
: crash, fixed-aviary1.0, fixed1.7.5, topcrash
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: Blake Kaplan (:mrbkap)
Mentors:
http://lcamtuf.coredump.cx/mangleme/g...
: 265102 265206 265946 265966 (view as bug list)
Depends on:
Blocks: Zalewski
  Show dependency treegraph
 
Reported: 2004-10-18 14:10 PDT by WD
Modified: 2014-04-26 03:31 PDT (History)
20 users (show)
dveditz: blocking1.7.5+
dveditz: blocking‑aviary1.0+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.83 KB, patch)
2004-10-18 17:21 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
patch for checkin (1.85 KB, patch)
2004-10-18 18:19 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
attempted patch (33.45 KB, patch)
2004-10-19 16:13 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
attempted patch - take 2 (3.22 KB, patch)
2004-10-19 16:20 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
attempted patch - take 2 (3.22 KB, patch)
2004-10-19 16:20 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Add a closing bracket to the 1.7 branch patch (2.36 KB, patch)
2004-10-20 02:30 PDT, Peter Weilbacher
no flags Details | Diff | Splinter Review
removed the "if (theChar)" from the 1.7 branch patch (3.21 KB, patch)
2004-10-20 03:55 PDT, Peter Weilbacher
mozilla: review+
bzbarsky: superreview+
mozilla: approval‑aviary+
mozilla: approval1.7.5+
Details | Diff | Splinter Review

Description WD 2004-10-18 14:10:30 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910

Visit the URL for this bug.   Mozilla crashes.

Reproducible: Always
Steps to Reproduce:
1.Go to http://lcamtuf.coredump.cx/mangleme/gallery/mozilla_die1.html
2.
3.

Actual Results:  
Crash

Expected Results:  
No crash

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB1379160K
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB1380495E
Comment 1 Christian :Biesinger (don't email me, ping me on IRC) 2004-10-18 14:59:05 PDT
see also bug 264944... that one seems a bit broader, marking dependency
Comment 2 Blake Kaplan (:mrbkap) 2004-10-18 17:21:02 PDT
Created attachment 162530 [details] [diff] [review]
patch v1

Allows NUL characters to stop our parsing of "tag identifiers". Also strips
these characters out of the input stream (at least after tag identifiers) since
attributes have a field day with these NULs (and we *really* don't want NULs
floating around attribute code).
Comment 3 Blake Kaplan (:mrbkap) 2004-10-18 18:19:17 PDT
Created attachment 162536 [details] [diff] [review]
patch for checkin

Updated to review comments.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2004-10-18 18:32:29 PDT
Comment on attachment 162536 [details] [diff] [review]
patch for checkin

r+sr=bzbarsky
Comment 5 Blake Kaplan (:mrbkap) 2004-10-18 18:52:17 PDT
Fix checked in:
Checking in src/nsScanner.cpp;
/cvsroot/mozilla/parser/htmlparser/src/nsScanner.cpp,v  <--  nsScanner.cpp
new revision: 3.136; previous revision: 3.135
done
Comment 6 Adam Hauner 2004-10-19 01:07:35 PDT
F10PR and actual Firefox branch nightbuild is also crashing:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB1389820E
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB1389919E
M1.7 on branch will probably also crash. 

mkaply, chofmann: could you consider preparing patch for branches, as this bug
was published in Bugtraq
<http://www.securityfocus.com/archive/1/378632/2004-10-15/2004-10-21/0>?
Comment 7 Mike Kaply [:mkaply] 2004-10-19 05:23:37 PDT
I'll certainly take this for the branches.

Does it apply as is?
Comment 8 Bill Mason 2004-10-19 10:04:02 PDT
*** Bug 265102 has been marked as a duplicate of this bug. ***
Comment 9 timeless 2004-10-19 12:59:00 PDT
mkaply@us.ibm.com: no it does not apply to 17 branch as is. the file lives in a
different place and the code is different.
Comment 10 Mike Kaply [:mkaply] 2004-10-19 13:48:13 PDT
Yeah, 1.7 code isn't even close. Anyone want to volunteer?
Comment 11 Blake Kaplan (:mrbkap) 2004-10-19 16:13:35 PDT
Created attachment 162629 [details] [diff] [review]
attempted patch

I'm not sure if this patch even compiles, but this is the last patch backported
to the 1.7 branch (I hope). Please test to make sure this works.
Comment 12 Blake Kaplan (:mrbkap) 2004-10-19 16:20:18 PDT
Created attachment 162632 [details] [diff] [review]
attempted patch - take 2

Sorry, got the wrong file last time. Also included a change to a function I
missed earlier. This needs testing, also.
Comment 13 Blake Kaplan (:mrbkap) 2004-10-19 16:20:24 PDT
Created attachment 162633 [details] [diff] [review]
attempted patch - take 2

Sorry, got the wrong file last time. Also included a change to a function I
missed earlier. This needs testing, also.
Comment 14 Daniel Veditz [:dveditz] 2004-10-20 01:54:06 PDT
marking blocking 1.7x, presuming aviary will follow suit and marking so.
Comment 15 Bernd 2004-10-20 02:28:32 PDT
*** Bug 265206 has been marked as a duplicate of this bug. ***
Comment 16 Peter Weilbacher 2004-10-20 02:30:22 PDT
Created attachment 162681 [details] [diff] [review]
Add a closing bracket to the 1.7 branch patch

The last 1.7 branch patch was missing the closing "}" for "if (theChar)" which
I added after "current++;". With this patch a build of the current 1.7 branch
does not crash any more, but instead it hangs with 100% CPU usage on the
testcase page [Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.7.3)
Gecko/20041020].
Comment 17 Peter Weilbacher 2004-10-20 03:55:29 PDT
Created attachment 162686 [details] [diff] [review]
removed the "if (theChar)" from the 1.7 branch patch

That was stupid of course, the "if" was meant to go as I could have seen in the
patch for the trunk. With this one it works fine for a 1.7 branch build on
OS/2.
Comment 18 Mike Kaply [:mkaply] 2004-10-20 05:49:21 PDT
Comment on attachment 162686 [details] [diff] [review]
removed the "if (theChar)" from the 1.7 branch patch

r=mkaply
a=mkaply for branches
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2004-10-20 08:24:47 PDT
Comment on attachment 162686 [details] [diff] [review]
removed the "if (theChar)" from the 1.7 branch patch

sr=bzbarsky.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2004-10-21 07:39:37 PDT
Note that another (less correct) fix for this was checked in on the branches in
bug 265068... (which was security-sensitive, so we had no idea it existed).
Comment 21 Jacek Piskozub 2004-10-21 08:29:55 PDT
Why is this bug "resolved fixed"??? This patch went nowhere. Certainly neither
to the trunk, nor to the 1.7 branch.

The thing checked in to the trunk is the much less comprehensive patch from bug
265068 which actually conflicts with this patch (it should be backed out first
as noted in comment 265068#13)

I do not reopen yet but if nothing happens soon...
Comment 22 Blake Kaplan (:mrbkap) 2004-10-21 08:51:11 PDT
(In reply to comment #21)
> Why is this bug "resolved fixed"??? This patch went nowhere. Certainly neither
> to the trunk, nor to the 1.7 branch.

The patch for the trunk was checked in by me, as I said in comment 5. If you
want to reopen until the patch for the branches is checked in (which will
probably be the moment the tree is reopened from the smoketest blockers), feel
free to.
Comment 23 Blake Kaplan (:mrbkap) 2004-10-21 08:54:18 PDT
(In reply to comment #22)
> The patch for the trunk was checked in by me, as I said in comment 5. If you
> want to reopen until the patch for the branches is checked in (which will
> probably be the moment the tree is reopened from the smoketest blockers), feel
> free to.

Sorry for the bugspam. I misspoke the avairy tree is *not* closed. This patch
will probably be checked in once the patch from bug 265068 is backed out.
Comment 24 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-21 09:55:58 PDT
Fix landed on branches. Too bad I didn't find this bug before landing the fix
for bug 265068 :(
Comment 25 Christian :Biesinger (don't email me, ping me on IRC) 2004-10-21 13:49:19 PDT
note: keyword for current 1.7 branch is fixed1.7.x. fixed1.7 was for bugs fixed
in the 1.7.0 release.

note2: bugs are not usually reopened just because they aren't landed on some
branch yet. the FIXED resolution applies to trunk.
Comment 26 Jacek Piskozub 2004-10-21 14:00:53 PDT
OK. Sorry. I (wrongly) thought the trunk patch was not applied. I did not check
far back enough :-(

But I did not reopen anyway :-)
Comment 27 Christian Eyrich 2004-10-22 16:57:41 PDT
(In reply to comment #17)
> Created an attachment (id=162686)
> removed the "if (theChar)" from the 1.7 branch patch

I'm sorry if this is spam, but is it ok the patch for the trunk covers only the
first half of this branch patch?
E.g. it doesn't remove the if(theChar) in ReadEntityIdentifier().

Comment 28 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-22 17:42:37 PDT
ReadEntityIdentifier() doesn't exist on the trunk, so AFAICT, it's all good...
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-23 11:00:41 PDT
(In reply to comment #28)
> ReadEntityIdentifier() doesn't exist on the trunk, so AFAICT, it's all good...

Ahem, I meant to say "aviary branch" there, not "trunk".
Comment 30 Erik Fabert 2004-10-25 08:11:25 PDT
*** Bug 265966 has been marked as a duplicate of this bug. ***
Comment 31 Bernard Alleysson 2004-10-25 09:52:36 PDT
*** Bug 265946 has been marked as a duplicate of this bug. ***
Comment 32 Jay Patel [:jay] 2004-10-27 13:02:28 PDT
Verifying this fix per Talkback data (no crashes since 20041020 builds) and the
fact that the repro testcase no longer crashes. Adding topcrash info for future
reference.  
Comment 33 Ido Kanner 2004-10-27 13:29:11 PDT
(In reply to comment #32)
> Verifying this fix per Talkback data (no crashes since 20041020 builds) and the
> fact that the repro testcase no longer crashes. Adding topcrash info for future
> reference.  
> 
On my new installation of Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3)
Gecko/20041026 Firefox/0.10.1 (Debian package  0.10.1+1.0PR-4)

it still crush... 
Comment 34 Ido Kanner 2004-10-27 15:13:04 PDT
(In reply to comment #33)
> (In reply to comment #32)
> > Verifying this fix per Talkback data (no crashes since 20041020 builds) and the
> > fact that the repro testcase no longer crashes. Adding topcrash info for future
> > reference.  
> > 
> On my new installation of Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3)
> Gecko/20041026 Firefox/0.10.1 (Debian package  0.10.1+1.0PR-4)
> 
> it still crush... 
> 

Replaying for myself...
Afther longer investigation, i found that the last upstream for the source was
in Oct 3. 

So my previous report may not be valid.
Comment 35 Daniel Veditz [:dveditz] 2004-10-27 21:55:05 PDT
Re: comment 34

The curse of Gecko dates being build dates rather than pull dates strikes again
:-(  The true 20041026 builds should be labelled Firefox/1.0RC1

I swear I'm going to fix that one of these days
Comment 36 Ido Kanner 2004-10-29 16:36:54 PDT
(In reply to comment #35)
> Re: comment 34
> 
> The curse of Gecko dates being build dates rather than pull dates strikes again
> :-(  The true 20041026 builds should be labelled Firefox/1.0RC1
> 
> I swear I'm going to fix that one of these days
On the new update of: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3)
Gecko/20041029 
that was made from the upstrem Thu, 28 Oct 2004, I can confirm that the bug
seems to be fixed
Comment 37 Jay Patel [:jay] 2004-12-13 16:47:33 PST
Adding M173 since this was a topcrasher for Mozilla 1.7.3 and to make sure I
verify this is gone once 1.7.5 is out the door.

Note You need to log in before you can comment on or make changes to this bug.