Malformed HTML document causes crash - M173 FF10PR1 [@ RuleProcessorData::RuleProcessorData]

VERIFIED FIXED

Status

()

Core
HTML: Parser
--
critical
VERIFIED FIXED
13 years ago
3 years ago

People

(Reporter: WD, Assigned: mrbkap)

Tracking

(4 keywords)

Trunk
crash, fixed-aviary1.0, fixed1.7.5, topcrash
Points:
---
Bug Flags:
blocking1.7.5 +
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:dos], crash signature, URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

13 years ago
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
see also bug 264944... that one seems a bit broader, marking dependency
Blocks: 264944

Updated

13 years ago
Keywords: crash
(Assignee)

Comment 2

13 years ago
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).
Assignee: parser → mrbkap
Status: NEW → ASSIGNED
(Assignee)

Updated

13 years ago
Attachment #162530 - Flags: superreview?(bzbarsky)
Attachment #162530 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

13 years ago
Created attachment 162536 [details] [diff] [review]
patch for checkin

Updated to review comments.
Attachment #162530 - Attachment is obsolete: true
Attachment #162530 - Flags: superreview?(bzbarsky)
Attachment #162530 - Flags: superreview-
Attachment #162530 - Flags: review?(bzbarsky)
Attachment #162530 - Flags: review-
Comment on attachment 162536 [details] [diff] [review]
patch for checkin

r+sr=bzbarsky
Attachment #162536 - Flags: superreview+
Attachment #162536 - Flags: review+
(Assignee)

Comment 5

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 6

13 years ago
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

13 years ago
I'll certainly take this for the branches.

Does it apply as is?

Comment 8

13 years ago
*** Bug 265102 has been marked as a duplicate of this bug. ***

Comment 9

13 years ago
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

13 years ago
Yeah, 1.7 code isn't even close. Anyone want to volunteer?
(Assignee)

Comment 11

13 years ago
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.
(Assignee)

Comment 12

13 years ago
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.
(Assignee)

Comment 13

13 years ago
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.
Attachment #162629 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #162632 - Attachment is obsolete: true
marking blocking 1.7x, presuming aviary will follow suit and marking so.
Flags: blocking1.7.x+
Flags: blocking-aviary1.0+
Whiteboard: [sg:dos] have patch, need branch reviews

Comment 15

13 years ago
*** Bug 265206 has been marked as a duplicate of this bug. ***

Comment 16

13 years ago
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].
Attachment #162633 - Attachment is obsolete: true

Comment 17

13 years ago
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.
Attachment #162681 - Attachment is obsolete: true

Comment 18

13 years ago
Comment on attachment 162686 [details] [diff] [review]
removed the "if (theChar)" from the 1.7 branch patch

r=mkaply
a=mkaply for branches
Attachment #162686 - Flags: superreview?(bzbarsky)
Attachment #162686 - Flags: review+
Attachment #162686 - Flags: approval1.7.x+
Attachment #162686 - Flags: approval-aviary+
Comment on attachment 162686 [details] [diff] [review]
removed the "if (theChar)" from the 1.7 branch patch

sr=bzbarsky.
Attachment #162686 - Flags: superreview?(bzbarsky) → superreview+

Updated

13 years ago
Whiteboard: [sg:dos] have patch, need branch reviews → [sg:dos] have patch, needs branch landing
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

13 years ago
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...
(Assignee)

Comment 22

13 years ago
(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.
(Assignee)

Comment 23

13 years ago
(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.
Fix landed on branches. Too bad I didn't find this bug before landing the fix
for bug 265068 :(
Keywords: fixed-aviary1.0, fixed1.7
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.
Keywords: fixed1.7 → fixed1.7.x

Comment 26

13 years ago
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

13 years ago
(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().

ReadEntityIdentifier() doesn't exist on the trunk, so AFAICT, it's all good...
(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".

Updated

13 years ago
OS: Windows XP → All
Hardware: PC → All

Comment 30

13 years ago
*** Bug 265966 has been marked as a duplicate of this bug. ***

Comment 31

13 years ago
*** Bug 265946 has been marked as a duplicate of this bug. ***

Comment 32

13 years ago
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.  
Status: RESOLVED → VERIFIED
Keywords: topcrash
Summary: Malformed HTML document causes crash [@RuleProcessorData::RuleProcessorData] → Malformed HTML document causes crash - FF10PR1 [@ RuleProcessorData::RuleProcessorData]

Comment 33

13 years ago
(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

13 years ago
(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.
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
Whiteboard: [sg:dos] have patch, needs branch landing → [sg:dos]

Comment 36

13 years ago
(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

13 years ago
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.
Summary: Malformed HTML document causes crash - FF10PR1 [@ RuleProcessorData::RuleProcessorData] → Malformed HTML document causes crash - M173 FF10PR1 [@ RuleProcessorData::RuleProcessorData]
Crash Signature: [@ RuleProcessorData::RuleProcessorData]
You need to log in before you can comment on or make changes to this bug.