1205177 - Coverity 1323780 indicates a dereference after null check

Status: RESOLVED FIXED

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

Description

[u493164]

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

Comment 1

[Abe - QA (:Abe_LV)]

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

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Bogdan, could you help here? Thanks

Comment 4

[Andi [:andi]]

Sure i'll update here when i have something.

Comment 5

[Abe - QA (:Abe_LV)]

It is confirmed that the issue exists. 
Andrew McCreight and Mike Conley, thanks for taking time on this issue.

Comment 6

[Andi [:andi]]

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();
>>      }
>>    }
>>  }

Comment 7

[Andi [:andi]]

Attachment: MozReview Request: Bug 1205177 - call fileHandleQueue->Finish if aFinish in FileHandleThreadPool::Enqueue. r?jst

Review commit: https://reviewboard.mozilla.org/r/34553/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34553/

Comment 8

[Andi [:andi]]

Note regarding the patch: i didn't check if fileHandleQueue not null since we use an infallible allocator.

Comment 9

[Johnny Stenback (:jst)]

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.

Comment 10

[Jan Varga [:janv]]

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.

Comment 11

[Andi [:andi]]

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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7112d0fae4

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a7112d0fae4