Closed Bug 1279560 Opened 4 years ago Closed 4 years ago

(coverity) use after free: mailnews/base/util/nsMsgDBFolder.cpp: |set| is returned after deleted (!)

Categories

(MailNews Core :: Database, defect, major)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 50.0

People

(Reporter: ishikawa, Assigned: ishikawa)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: CID 1137669)

Attachments

(1 file, 1 obsolete file)

Coverity found this:
|set| is deleted in one if-statement, but in the end returned.

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#5985

5985/* static */ nsMsgKeySetU* nsMsgKeySetU::Create()
5986{
5987  nsMsgKeySetU* set = new nsMsgKeySetU;
    1. Condition set, taking true branch
5988  if (set)
5989  {
5990    set->loKeySet = nsMsgKeySet::Create();
5991    set->hiKeySet = nsMsgKeySet::Create();
5992  }
    2. Condition set, taking true branch
    3. Condition set->loKeySet, taking false branch
5993  if (!(set && set->loKeySet && set->hiKeySet))
    4. freed_arg: operator delete frees set. [Note: The source code implementation of the function has been overridden by a builtin model.]
5994    delete set;
    CID 1137669 (#1 of 1): Use after free (USE_AFTER_FREE)5. use_after_free: Using freed pointer set.
5995  return set;
5996}
5997

Observation:

I think we should return nullptr when |delete set| on line 5994 is invoked.
Assignee: nobody → ishikawa
Comment on attachment 8762568 [details] [diff] [review]
Bug 127960 - set |set| to nullptr after |delete set| before |return set|

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

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +5989,5 @@
>    {
>      set->loKeySet = nsMsgKeySet::Create();
>      set->hiKeySet = nsMsgKeySet::Create();
>    }
>    if (!(set && set->loKeySet && set->hiKeySet))

I wonder why we test !set again, when we already tested above that is is not null. Would you agree this test could be put inside the if() above and drop !set from it?
(In reply to :aceman from comment #3)
> Comment on attachment 8762568 [details] [diff] [review]
> Bug 127960 - set |set| to nullptr after |delete set| before |return set|
> 
> Review of attachment 8762568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/base/util/nsMsgDBFolder.cpp
> @@ +5989,5 @@
> >    {
> >      set->loKeySet = nsMsgKeySet::Create();
> >      set->hiKeySet = nsMsgKeySet::Create();
> >    }
> >    if (!(set && set->loKeySet && set->hiKeySet))
> 
> I wonder why we test !set again, when we already tested above that is is not
> null. Would you agree this test could be put inside the if() above and drop
> !set from it?

You mean like the new patch.

I put Joshua as reviewer. If this is not appropriate, please change this.

TIA
Attachment #8762568 - Attachment is obsolete: true
Attachment #8762603 - Flags: review?(Pidgeot18)
Comment on attachment 8762603 [details] [diff] [review]
Bug 127960 - set |set| to nullptr after |delete set| before |return set|

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

Yes, thanks.
I think we do not need to bother jcranmer with this one.
Attachment #8762603 - Flags: review?(Pidgeot18) → review+
https://hg.mozilla.org/comm-central/rev/7cb60a63a47e7c77fce8cc07d2d142fb3dd06f21

I didn't notice the checkin comment contained incorrect bug number. 127960 instead of 1279560.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
(In reply to :aceman from comment #6)
> https://hg.mozilla.org/comm-central/rev/
> 7cb60a63a47e7c77fce8cc07d2d142fb3dd06f21
> 
> I didn't notice the checkin comment contained incorrect bug number. 127960
> instead of 1279560.

Aha, thank you for noticing this. I think I need a more or less automated way to
copy coverity issue to bugzilla and then create a patch based on correct bugzilla # in checkin comment, etc.
You need to log in before you can comment on or make changes to this bug.