459496 - nsNavHistory misses checks on pointer arguments on a couple of places

Status: RESOLVED DUPLICATE of bug 489872

(Firefox :: Bookmarks & History, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 1

[Marco Bonardo [:mak]]

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)

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 3

[(no longer active)]

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

Comment 4

[Marco Bonardo [:mak]]

(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

Comment 5

[Shawn Wilsher :sdwilsh]

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?

Comment 6

[(no longer active)]

I think it's still worth it, because these checks are not so expensive, right?

Comment 7

[Shawn Wilsher :sdwilsh]

It's a possible branch every time you enter the function.

Comment 8

[(no longer active)]

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

Comment 9

[Shawn Wilsher :sdwilsh]

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

Comment 10

[Ed Lee :Mardak]

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?

Comment 11

[(no longer active)]

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

Comment 12

[Gervase Markham [:gerv]]

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