nsNavHistory misses checks on pointer arguments on a couple of places

RESOLVED DUPLICATE of bug 489872

Status

()

Firefox
Bookmarks & History
--
major
RESOLVED DUPLICATE of bug 489872
9 years ago
8 years ago

People

(Reporter: whimboo, Assigned: Away for a while)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081009 Minefield/3.1b2pre ID:20081009020344

Member functions of nsNavHistory mostly don't use NS_ENSURE_ARG_POINTER to check if we have a valid argument before an assignment:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#650

This could lead into crashes e.g. if extensions pass invalid arguments.

Ehsan said, he want to contribute a patch. So lets assign this bug to him directly.
yes, this is something we need, not all methods have correct input checks, thanks

(PS: we are actually still working on fsync patches, so this patch could probably bitrot upon that and be taken after fsync landing)
Looks like it's not only related to nsNavHistory.cpp. Also other files in toolkit/components/places/src/ leaks this check. Shall we handle all these in one bug or should it be split up in different ones?
(Assignee)

Comment 3

9 years ago
(In reply to comment #1)
> (PS: we are actually still working on fsync patches, so this patch could
> probably bitrot upon that and be taken after fsync landing)

OK.  However, I'm wondering if there's a tracking bug for landing the fsync work (there are a few individual ones you mentioned before) so that I can get notified when that work is finished.  Alternatively, I could just keep an eye on the planet...

(In reply to comment #2)
> Looks like it's not only related to nsNavHistory.cpp. Also other files in
> toolkit/components/places/src/ leaks this check. Shall we handle all these in
> one bug or should it be split up in different ones?

I guess it would be safe to keep them all in this bug, as far as I'm concerned.
(In reply to comment #3)
> OK.  However, I'm wondering if there's a tracking bug for landing the fsync
> work (there are a few individual ones you mentioned before) so that I can get
> notified when that work is finished. 

bug 442967 should be the last closed bug in the fsync chain
(Assignee)

Updated

9 years ago
Depends on: 442967
Anything passed in via JS should not need the NS_ENSURE_ARG_POINTER.  Do we really have that many c++ consumers that we have to worry about?
(Assignee)

Comment 6

9 years ago
I think it's still worth it, because these checks are not so expensive, right?
It's a possible branch every time you enter the function.
(Assignee)

Comment 8

9 years ago
If the condition (which is the invalidity of the pointer) is not true, the branch is skipped, but if it's false, it would prevent a (possibly crash) bug which will happen if the arguments check code is not in place...
(In reply to comment #8)
> If the condition (which is the invalidity of the pointer) is not true, the
> branch is skipped, but if it's false, it would prevent a (possibly crash) bug
> which will happen if the arguments check code is not in place...
That statement is *highly* dependent on what the compiler generates...
Depends on: 489872

Comment 10

9 years ago
FYI, NS_ENSURE_ARG uses NS_ENSURE_TRUE which uses NS_UNLIKELY which can hint to the compiler that anything down that unlikely path shouldn't steal register allocation spots and if the hardware supports it, adding static prediction branch bits.

Is this bug still needed now that 489872 is fixed?
(Assignee)

Comment 11

9 years ago
(In reply to comment #10)
> Is this bug still needed now that 489872 is fixed?

No, I think this bug was meant to be like that bug!
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 489872
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.