Closed Bug 1234542 Opened 4 years ago Closed 4 years ago

[Static Analysis][Dereference null return value] In function nsPersistentProperties::SetStringProperty from nsPersistentProperties.cpp

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1324685 )

Attachments

(1 file, 3 obsolete files)

The Static Analysis tool Coverity added entry can be null so causing a null pointer dereference:

>>  auto entry = static_cast<PropertyTableEntry*>
>>                          (mTable.Add(flatKey.get(), mozilla::fallible));
>>
>>  if (entry->mKey) {

entry can be null only if mTable.Add fails, and this can happen if malloc fails due to oom:

>>    mEntryStore.Set((char*)malloc(nbytes));
>>    if (!mEntryStore.Get()) {
>>      return nullptr;
>>    }
Attached patch Bug 1234542.diff (obsolete) — Splinter Review
Attachment #8701044 - Flags: review?(nfroyd)
Comment on attachment 8701044 [details] [diff] [review]
Bug 1234542.diff

Review of attachment 8701044 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking at this.

::: xpcom/ds/nsPersistentProperties.cpp
@@ +526,5 @@
>    auto entry = static_cast<PropertyTableEntry*>
>                            (mTable.Add(flatKey.get(), mozilla::fallible));
>  
> +  if (!entry)
> +      return NS_ERROR_FAILURE;

I agree this would fix Coverity complaining about null pointer dereferences.  I'm not sure this is the best fix, as it turns a very visible error (a crash) into a silent error (nothing really checks the return value of SetStringProperty).  Fixing up everything to properly check the return value of SetStringProperty looks...difficult.

I think we want to preserve the crashiness here, so I would be OK making that Add() call infallible instead...that will at least get things onto the OOM radar, if such a thing exists.
Attachment #8701044 - Flags: review?(nfroyd)
Attached patch Bug 1234542.diff (obsolete) — Splinter Review
So i've removed the null check, now we call that Add function that when oom tiggers a NS_ABORT_OOM
Attachment #8701044 - Attachment is obsolete: true
Attachment #8703577 - Flags: review?(nfroyd)
Attachment #8703577 - Flags: review?(nfroyd) → review+
Keywords: checkin-needed
Could you please update the commit message for the patch, since it no longer reflects what the patch actually does?
Flags: needinfo?(bogdan.postelnicu)
Keywords: checkin-needed
Attached patch Bug 1234542.diff (obsolete) — Splinter Review
Attachment #8703577 - Attachment is obsolete: true
Flags: needinfo?(bogdan.postelnicu)
Attachment #8703613 - Flags: review?(nfroyd)
Comment on attachment 8703613 [details] [diff] [review]
Bug 1234542.diff

Review of attachment 8703613 [details] [diff] [review]:
-----------------------------------------------------------------

How about "don't use fallible Add in SetStringProperty"?  r=me with that.

"added infallible Add function" isn't correct, because you're not adding any new code, you're using a different function.  There's no need for the "that when it fails..." part, because it's understood that is the point of using an infallible call.
Attachment #8703613 - Flags: review?(nfroyd) → review+
Attached patch Bug 1234542.diffSplinter Review
Attachment #8703613 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0752386f7d96
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.