Crash [@ nsGlobalHistory::IsURLInHistory] null pointer dereference

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
History: Global
--
trivial
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Mook, Assigned: Mook)

Tracking

({fixed-seamonkey1.1.8, fixed1.8.1.12})

Trunk
mozilla1.9beta2
fixed-seamonkey1.1.8, fixed1.8.1.12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
Created attachment 261093 [details] [diff] [review]
null check before calling (trivial one line patch)

(see URL)

If somebody passing a nsIRDFNode that is not a nsIRDFResource to nsGlobalHistory::GetSource, QI fails and null is passed to IsURLInHistory leading to null pointer dereference.

This was actually found via poking the Firefox 2.0.0.x history (in #extdev), but I'm assuming nobody would want to review non-Places history backend there.  So I checked the suite version manually - note though, I haven't actually attempted to reproduce this.

-- Code used to trigger: (from johnm on IRC)
  var RDF = Components.classes["@mozilla.org/rdf/rdf-service;1"]
                      .getService(Components.interfaces.nsIRDFService);
  var HDS  = RDF.GetDataSource("rdf:history");
  var urlArc = RDF.GetResource("http://home.netscape.com/NC-rdf#URL");
/* note that urltomatch should have been a resource, not a literal */
  var urltomatch = RDF.GetLiteral("http://www.google.com");
/* crashes on the next line */
  var subject = HDS.GetSource(urlArc, urltomatch, true);

Will attach patch, but I haven't actually tried to reproduce on suite yet... :)

Hmm, should I check in the caller, or callee?  All other callers do the check...
Attachment #261093 - Flags: superreview?(cbiesinger)
Attachment #261093 - Flags: review?(neil)

Updated

10 years ago
Summary: [crash] @ nsGlobalHistory::IsURLInHistory null pointer dereference → Crash [@ nsGlobalHistory::IsURLInHistory] null pointer dereference
Attachment #261093 - Flags: superreview?(cbiesinger) → superreview+

Comment 1

10 years ago
Comment on attachment 261093 [details] [diff] [review]
null check before calling (trivial one line patch)

Sorry for the delay, the bugmail for this request must have got lost.
Attachment #261093 - Flags: review?(neil) → review+
(Assignee)

Comment 2

10 years ago
Patch has r/sr, apparently Seamonkey-only doesn't need a+ yet.  (This is in xpfe, and besides Firefox is using Places)
Keywords: checkin-needed
Assignee: nobody → mook.moz+mozbz
Uh, what about mailnews? Doesn't it use nsGlobalHistory still? Until toolkit/components/history/src/nsGlobalHistory.cpp is cvs removed for non-use, I think you should patch it, too.
Note that branch still takes crash fixes, too, so you could probably get it on 1.8 branch as a stability fix.
(In reply to comment #3)
> Uh, what about mailnews? Doesn't it use nsGlobalHistory still?

Ignore this... however, the issue still stands. Let's fix toolkit, too.
Created attachment 290353 [details] [diff] [review]
toolkit version

Fix toolkit's version, too. This will be for branch, too.
Attachment #290353 - Flags: superreview?(cbiesinger)
Attachment #290353 - Flags: review?(neil)
Checking in xpfe/components/history/src/nsGlobalHistory.cpp;
/cvsroot/mozilla/xpfe/components/history/src/nsGlobalHistory.cpp,v  <--  nsGlobalHistory.cpp
new revision: 1.229; previous revision: 1.228
done
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All

Updated

10 years ago
Attachment #290353 - Flags: review?(neil) → review+
Attachment #290353 - Flags: superreview?(cbiesinger) → superreview+
Comment on attachment 290353 [details] [diff] [review]
toolkit version

Simple patch to fix null pointer dereference.
Attachment #290353 - Flags: approval1.9?
Attachment #290353 - Flags: approval1.8.1.12?

Updated

10 years ago
Attachment #290353 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in toolkit/components/history/src/nsGlobalHistory.cpp;
/cvsroot/mozilla/toolkit/components/history/src/nsGlobalHistory.cpp,v  <--  nsGlobalHistory.cpp
new revision: 1.86; previous revision: 1.85
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
Comment on attachment 290353 [details] [diff] [review]
toolkit version

approved for 1.8.1.12, a=dveditz for release-drivers

Won't SeaMonkey want the xpfe version on the branch?
Attachment #290353 - Flags: approval1.8.1.12? → approval1.8.1.12+
Keywords: checkin-needed
KaiRo / CTho: I can't request approval for the seamonkey branch directly on this patch. Can I get a verbal approval?
I'm not on the council any more.
Checking in toolkit/components/history/src/nsGlobalHistory.cpp;
/cvsroot/mozilla/toolkit/components/history/src/nsGlobalHistory.cpp,v  <--  nsGlobalHistory.cpp
new revision: 1.58.2.9; previous revision: 1.58.2.8
done
Keywords: checkin-needed → fixed1.8.1.12

Comment 14

10 years ago
Comment on attachment 261093 [details] [diff] [review]
null check before calling (trivial one line patch)

a=me for SeaMonkey 1.1.8
Checking in xpfe/components/history/src/nsGlobalHistory.cpp;
/cvsroot/mozilla/xpfe/components/history/src/nsGlobalHistory.cpp,v  <--  nsGlobalHistory.cpp
new revision: 1.211.2.4; previous revision: 1.211.2.3
done
Keywords: fixed-seamonkey1.1.8
You need to log in before you can comment on or make changes to this bug.