Closed Bug 264956 Opened 20 years ago Closed 20 years ago

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

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: wd, Assigned: mrbkap)

References

()

Details

(4 keywords, Whiteboard: [sg:dos])

Crash Data

Attachments

(2 files, 5 obsolete files)

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: Zalewski
Keywords: crash
Attached patch patch v1 (obsolete) — Splinter Review
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
Attachment #162530 - Flags: superreview?(bzbarsky)
Attachment #162530 - Flags: review?(bzbarsky)
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+
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
Closed: 20 years ago
Resolution: --- → FIXED
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>?
I'll certainly take this for the branches.

Does it apply as is?
*** Bug 265102 has been marked as a duplicate of this bug. ***
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.
Yeah, 1.7 code isn't even close. Anyone want to volunteer?
Attached patch attempted patch (obsolete) — Splinter Review
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.
Attached patch attempted patch - take 2 (obsolete) — Splinter Review
Sorry, got the wrong file last time. Also included a change to a function I
missed earlier. This needs testing, also.
Attached patch attempted patch - take 2 (obsolete) — Splinter Review
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
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
*** Bug 265206 has been marked as a duplicate of this bug. ***
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
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 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+
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).
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...
(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.
(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 :(
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.7fixed1.7.x
OK. Sorry. I (wrongly) thought the trunk patch was not applied. I did not check
far back enough :-(

But I did not reopen anyway :-)
(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".
OS: Windows XP → All
Hardware: PC → All
*** Bug 265966 has been marked as a duplicate of this bug. ***
*** Bug 265946 has been marked as a duplicate of this bug. ***
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]
(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... 
(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]
(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
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.