Last Comment Bug 376976 - Crash [@ nsGlobalHistory::IsURLInHistory] null pointer dereference
: Crash [@ nsGlobalHistory::IsURLInHistory] null pointer dereference
Status: RESOLVED FIXED
: fixed-seamonkey1.1.8, fixed1.8.1.12
Product: Core
Classification: Components
Component: History: Global (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla1.9beta2
Assigned To: :Mook
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-09 20:44 PDT by :Mook
Modified: 2007-12-19 04:38 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
null check before calling (trivial one line patch) (1.23 KB, patch)
2007-04-09 20:44 PDT, :Mook
neil: review+
cbiesinger: superreview+
Details | Diff | Review
toolkit version (1.08 KB, patch)
2007-11-26 23:35 PST, Reed Loden [:reed] (use needinfo?)
neil: review+
cbiesinger: superreview+
dveditz: approval1.8.1.12+
dsicore: approval1.9+
Details | Diff | Review

Description :Mook 2007-04-09 20:44:17 PDT
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...
Comment 1 neil@parkwaycc.co.uk 2007-11-22 08:22:37 PST
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 2007-11-26 22:08:54 PST
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] (use needinfo?) 2007-11-26 23:16:37 PST
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] (use needinfo?) 2007-11-26 23:18:11 PST
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] (use needinfo?) 2007-11-26 23:33:04 PST
(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] (use needinfo?) 2007-11-26 23:35:25 PST
Created attachment 290353 [details] [diff] [review]
toolkit version

Fix toolkit's version, too. This will be for branch, too.
Comment 7 Reed Loden [:reed] (use needinfo?) 2007-11-26 23:41:32 PST
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] (use needinfo?) 2007-11-28 14:33:28 PST
Comment on attachment 290353 [details] [diff] [review]
toolkit version

Simple patch to fix null pointer dereference.
Comment 9 Reed Loden [:reed] (use needinfo?) 2007-11-28 23:04:55 PST
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] 2007-12-17 15:52:56 PST
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] (use needinfo?) 2007-12-18 21:00:23 PST
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 cst@andrew.cmu.edu cst@yecc.com] 2007-12-18 21:11:29 PST
I'm not on the council any more.
Comment 13 Reed Loden [:reed] (use needinfo?) 2007-12-19 00:43:13 PST
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 (not working on stability any more) 2007-12-19 04:14:27 PST
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] (use needinfo?) 2007-12-19 04:38:16 PST
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

Note You need to log in before you can comment on or make changes to this bug.