Closed
Bug 1205177
Opened 9 years ago
Closed 9 years ago
Coverity 1323780 indicates a dereference after null check
Categories
(Core :: DOM: Core & HTML, defect)
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
Updated•9 years ago
|
Component: Untriaged → DOM
Product: Firefox → Core
Comment 1•9 years ago
|
||
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
Updated•9 years ago
|
Flags: needinfo?(calixte.denizet)
Comment 2•9 years ago
|
||
(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.
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•9 years ago
|
||
Bogdan, could you help here? Thanks
Flags: needinfo?(calixte.denizet) → needinfo?(bogdan.postelnicu)
Assignee | ||
Comment 4•9 years ago
|
||
Sure i'll update here when i have something.
Flags: needinfo?(bogdan.postelnicu)
Comment 5•9 years ago
|
||
It is confirmed that the issue exists. Andrew McCreight and Mike Conley, thanks for taking time on this issue.
Assignee | ||
Comment 6•9 years ago
|
||
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 | ||
Comment 7•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/34553/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34553/
Attachment #8718388 -
Flags: review?(jst)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bogdan.postelnicu
Assignee | ||
Comment 8•9 years ago
|
||
Note regarding the patch: i didn't check if fileHandleQueue not null since we use an infallible allocator.
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
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 13•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9a7112d0fae4
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•8 years ago
|
Blocks: coverity-analysis
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•