Closed Bug 1205177 Opened 4 years ago Closed 4 years ago

Coverity 1323780 indicates a dereference after null check

Categories

(Core :: DOM: Core & HTML, defect)

43 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: calixte.denizet, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1323780])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20150817204646

Steps to reproduce:

Coverity indicates a null check for existingFileHandleQueue at [1] and the pointer is derefenced when null [2].   

[1] https://dxr.mozilla.org/mozilla-central/source/dom/filehandle/ActorsParent.cpp#866
[2] https://dxr.mozilla.org/mozilla-central/source/dom/filehandle/ActorsParent.cpp#900
Keywords: coverity
Whiteboard: [CID 1323780]
Component: Untriaged → DOM
Product: Firefox → Core
Version 	47.0a1
Build ID 	20160209030347
Update Channel 	nightly
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0

Hi Calixte,
Thank you for taking time to report this.
Are you still able to reproduce this in the latest versions ?  Can you give more detail on how to reproduce this? Like attachments or any inputs on how to use the given url.thanks
Flags: needinfo?(calixte.denizet)
(In reply to Abe - QA from comment #1)
> Thank you for taking time to report this.
> Are you still able to reproduce this in the latest versions ?  Can you give
> more detail on how to reproduce this? Like attachments or any inputs on how
> to use the given url.thanks

This is a static analysis bug based on Coverity reports. There is no test case. Somebody just needs to look at the source code and see if it still applies.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Bogdan, could you help here? Thanks
Flags: needinfo?(calixte.denizet) → needinfo?(bogdan.postelnicu)
Sure i'll update here when i have something.
Flags: needinfo?(bogdan.postelnicu)
It is confirmed that the issue exists. 
Andrew McCreight and Mike Conley, thanks for taking time on this issue.
At a quick look this can lead to a null pointer dereference if in function  that returns the pointer copied to existingFileHandleQueue:

>>auto
>>FileHandleThreadPool::
>>DirectoryInfo::GetFileHandleQueue(FileHandle* aFileHandle) -> FileHandleQueue*
>>{
>>  uint32_t count = mFileHandleQueues.Length();
>>  for (uint32_t index = 0; index < count; index++) {
>>    RefPtr<FileHandleQueue>& fileHandleQueue = mFileHandleQueues[index];
>>    if (fileHandleQueue->mFileHandle == aFileHandle) {
>>      return fileHandleQueue;
>>    }
>>  }
>>  return nullptr;
>>}

mFileHandleQueues is empty thus count = 0 leading to return nullptr or second case aFileHandle is not present in that queue. 

Now following the logic from here on:

>>  if (existingFileHandleQueue) {
>>    existingFileHandleQueue->Enqueue(aFileHandleOp);
>>    if (aFinish) {
>>      existingFileHandleQueue->Finish();
>>    }
>>    return;
>>  }

i think the correct resolution is as follows:

>>  else {
>>    FileHandleQueue* fileHandleQueue =
>>      directoryInfo->CreateFileHandleQueue(aFileHandle);
>>
>>    if (aFileHandleOp) {
>>      fileHandleQueue->Enqueue(aFileHandleOp);
>>      if (aFinish) {
>>        fileHandleQueue->Finish();
>>      }
>>    }
>>  }
Assignee: nobody → bogdan.postelnicu
Note regarding the patch: i didn't check if fileHandleQueue not null since we use an infallible allocator.
Comment on attachment 8718388 [details]
MozReview Request: Bug 1205177 - call fileHandleQueue->Finish if aFinish in FileHandleThreadPool::Enqueue. r?jst

This seems correct to me, but Jan knows this code much better than I do so he should review this.
Attachment #8718388 - Flags: review?(jst) → review?(jvarga)
Comment on attachment 8718388 [details]
MozReview Request: Bug 1205177 - call fileHandleQueue->Finish if aFinish in FileHandleThreadPool::Enqueue. r?jst

https://reviewboard.mozilla.org/r/34553/#review31693

r=janv

::: dom/filehandle/ActorsParent.cpp:900
(Diff revision 1)
> -        existingFileHandleQueue->Finish();
> +        fileHandleQueue->Finish();

Hm, probably copy and paste mistake. Yeah, this change is correct. Thanks for the fix.
Attachment #8718388 - Flags: review?(jvarga) → review+
https://reviewboard.mozilla.org/r/34553/#review31693

> Hm, probably copy and paste mistake. Yeah, this change is correct. Thanks for the fix.

thx for the review.
https://hg.mozilla.org/mozilla-central/rev/9a7112d0fae4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.