376976 - Crash [@ nsGlobalHistory::IsURLInHistory] null pointer dereference

Status: RESOLVED FIXED

(Core Graveyard :: History: Global, defect)

Description

[:Mook]

Attachment: 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...

Comment 1

[[e-mail]]

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.

Comment 2

[:Mook]

Patch has r/sr, apparently Seamonkey-only doesn't need a+ yet.  (This is in xpfe, and besides Firefox is using Places)

Comment 3

[Reed Loden [:reed]]

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.

Comment 4

[Reed Loden [:reed]]

Note that branch still takes crash fixes, too, so you could probably get it on 1.8 branch as a stability fix.

Comment 5

[Reed Loden [:reed]]

(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.

Comment 6

[Reed Loden [:reed]]

Attachment: toolkit version

Fix toolkit's version, too. This will be for branch, too.

Comment 7

[Reed Loden [:reed]]

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

Comment 8

[Reed Loden [:reed]]

Comment on attachment 290353 [details] [diff] [review]
toolkit version

Simple patch to fix null pointer dereference.

Comment 9

[Reed Loden [:reed]]

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

Comment 10

[Daniel Veditz [:dveditz]]

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?

Comment 11

[Reed Loden [:reed]]

KaiRo / CTho: I can't request approval for the seamonkey branch directly on this patch. Can I get a verbal approval?

Comment 12

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I'm not on the council any more.

Comment 13

[Reed Loden [:reed]]

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

Comment 14

[Robert Kaiser]

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

a=me for SeaMonkey 1.1.8

Comment 15

[Reed Loden [:reed]]

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