Closed Bug 344787 Opened 15 years ago Closed 15 years ago

NULL pointer can be dereferenced in nsHTMLFormElement.cpp

Categories

(Core :: Layout: Form Controls, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: junkmailnotread, Assigned: mats)

References

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.4) Gecko/20060606 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.4) Gecko/20060606 Firefox/1.5.0.4

While reading the current revision (1.203) of nsHTMLFormElement.cpp:

content/html/content/src/nsHTMLFormElement.cpp

I noticed an instance where a NULL pointer can be dereferenced. Here's the code snippet with line numbers added:

   929    nsIDocument* doc = GetCurrentDoc();
   930    if (!doc) {
   931      // Nothing to do
   932    }
   933  
   934    nsCOMPtr<nsISupports> container = doc->GetContainer();

If GetCurrentDoc() returns a NULL pointer in line 929, and thus the test in line 930 evaluates to TRUE, then a NULL pointer will be dereferenced in line 934. This will obviously crash the process.

This bug is theoretical. It's possible that GetCurrentDoc() never returns a NULL pointer. However the test in line 930 suggests that it can.

Reproducible: Didn't try
if the function wasn't supposed to return null, it'd be called CurrentDoc() instead.

for reference, the code you're reading is from Core not Firefox.
firefox code is mostly mozilla/browser
toolkit code is mostly mozilla/toolkit
most other code is core.
Severity: normal → critical
Component: Form Manager → Layout: Form Controls
Product: Firefox → Core
QA Contact: form.manager → layout.form-controls
Version: unspecified → Trunk
Attached patch Patch rev. 1Splinter Review
This code was changed by bug 100533.
Attachment #229363 - Flags: superreview?(bzbarsky)
Attachment #229363 - Flags: review?(bzbarsky)
Attachment #229363 - Flags: superreview?(bzbarsky)
Attachment #229363 - Flags: superreview+
Attachment #229363 - Flags: review?(bzbarsky)
Attachment #229363 - Flags: review+
Mats, thanks for fixing this!  Please get it in on the 1.8 branches too?
Assignee: nobody → mats.palmgren
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Flags: blocking1.8.1? → blocking1.8.1+
(In reply to comment #3)
> Please get it in on the 1.8 branches too?

Will ask for approval after it has baked a couple of days.

Checked in to trunk at 2006-07-22 04:07 PDT.

-> FIXED
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 229363 [details] [diff] [review]
Patch rev. 1

Simple null-check fix, zero risk.
Attachment #229363 - Flags: approval1.8.1?
Attachment #229363 - Flags: approval1.8.0.6?
Comment on attachment 229363 [details] [diff] [review]
Patch rev. 1

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.
Attachment #229363 - Flags: approval1.8.1? → approval1.8.1+
It's not needed on the 1.8 branch yet, since bug 100533 hasn't landed there yet.
Blocks: 100533
Attachment #229363 - Flags: approval1.8.0.6?
Flags: blocking1.9a1?
Flags: blocking1.8.0.6?
No need to block 1.8.1 since 100533 hasn't landed there.
Flags: blocking1.8.1+
Comment on attachment 229363 [details] [diff] [review]
Patch rev. 1

Clearing a flag since bug 100533 never landed on 1.8.1 branch and thus this shouldn't
Attachment #229363 - Flags: approval1.8.1+
You need to log in before you can comment on or make changes to this bug.