Closed Bug 1226526 Opened 9 years ago Closed 9 years ago

[Static Analysis][Called C++ object pointer is null] GetIOService from netwerk/protocol/http/nsHttpHandler.cpp Called C++ object pointer is null

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: clang-analyzer)

User Story

It seems that maybe my first comment was to brief, scan-build stated that result is dereferenced without being null checked in GetIOService():

>> nsHttpHandler::GetIOService(nsIIOService** result)
>> {
>>     NS_ADDREF(*result = mIOService);
>>     return NS_OK;
>> } 

Looking through the code GetIOService is called with valid pointers:

>>  nsCOMPtr<nsIIOService> ioService;
>>  rv = gHttpHandler->GetIOService(getter_AddRefs(ioService));
>>  NS_ENSURE_SUCCESS(rv, rv);


But to be future proof we should assert the pointer.

Attachments

(2 files)

Bug 1226526.diff
760 bytes, patch
Details | Diff | Splinter Review
MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com
58 bytes, text/x-review-board-request
mcmanus
: review+
andi
: review+
Details
The Static Analysis tool Scan-Build added a null pointer error in GetIOService from netwerk/protocol/http/nsHttpHandler.cpp.
Attached patch Bug 1226526.diff — — Splinter Review
Assignee: nobody → bogdan.postelnicu
Summary: [Static Analysis][alled C++ object pointer is null] GetIOService from netwerk/protocol/http/nsHttpHandler.cpp Called C++ object pointer is null → [Static Analysis][Called C++ object pointer is null] GetIOService from netwerk/protocol/http/nsHttpHandler.cpp Called C++ object pointer is null
User Story: (updated)
Comment on attachment 8706836 [details] MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30501/diff/1-2/
Attachment #8706836 - Attachment description: MozReview Request: Bug 1226526 - added moz_assert on result, in order to check it's validity. r?pmcmanus@mozilla.com → MozReview Request: Bug 1226526 - added moz_assert on result, in order to check it's validity. r?mcmanus@ducksong.com
Attachment #8706836 - Flags: review?(mcmanus)
Attachment #8706836 - Flags: review?(mcmanus)
Comment on attachment 8706836 [details] MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com https://reviewboard.mozilla.org/r/30501/#review27249 r- ::: netwerk/protocol/http/nsHttpHandler.cpp:552 (Diff revision 2) > + MOZ_ASSERT(result); please explain why this is helpful. If result is NULL you will get a crash with the same signature one line later - the check doesn't add any value. If you wanted to do a runtime if (!result) return NS_ERROR_UNEXPECTED that would be ok
That assert wouldn't have stopped the crash, would have just silenced Coverity.
Attachment #8706836 - Attachment description: MozReview Request: Bug 1226526 - added moz_assert on result, in order to check it's validity. r?mcmanus@ducksong.com → MozReview Request: Bug 1226526 - check to see if result is valid pointer, if not return error code. r?mcmanus@ducksong.com
Attachment #8706836 - Flags: review?(mcmanus)
Comment on attachment 8706836 [details] MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30501/diff/2-3/
https://reviewboard.mozilla.org/r/30501/#review27249 > please explain why this is helpful. If result is NULL you will get a crash with the same signature one line later - the check doesn't add any value. > > If you wanted to do a runtime if (!result) return NS_ERROR_UNEXPECTED that would be ok did the modification that you suggested, the assert was added jsut to silence Coverity.
(In reply to Bogdan Postelnicu from comment #7) > did the modification that you suggested, the assert was added jsut to > silence Coverity. In general, that is the tail wagging the dog. Fix the tools (or the post processing of the tools).
just to correct something that i've said earlier, by mistake i was refering to Coverity, this issues has been reported by scan-build, as i said in the first post.
Comment on attachment 8706836 [details] MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com https://reviewboard.mozilla.org/r/30501/#review27289 I'll take it if its updated to ns_ensure_arg_pointer.. I should have mentioned that the first time. sorry. r- ::: netwerk/protocol/http/nsHttpHandler.cpp:553 (Diff revisions 2 - 3) > + return NS_ERROR_UNEXPECTED; I wasn't specific enough. This should be NS_ENSURE_ARG_POINTER (result);
Attachment #8706836 - Flags: review?(mcmanus)
Comment on attachment 8706836 [details] MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30501/diff/3-4/
Attachment #8706836 - Attachment description: MozReview Request: Bug 1226526 - check to see if result is valid pointer, if not return error code. r?mcmanus@ducksong.com → MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r?mcmanus@ducksong.com
Attachment #8706836 - Flags: review?(mcmanus)
https://reviewboard.mozilla.org/r/30501/#review27289 no problem, i've updated the patch.
Comment on attachment 8706836 [details] MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com https://reviewboard.mozilla.org/r/30501/#review27299
Attachment #8706836 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Comment on attachment 8706836 [details] MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com https://reviewboard.mozilla.org/r/30501/#review27487 Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com
Attachment #8706836 - Flags: review+
Comment on attachment 8706836 [details] MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30501/diff/4-5/
Attachment #8706836 - Attachment description: MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r?mcmanus@ducksong.com → MozReview Request: Bug 1226526 - NS_ENSURE_ARG_POINTER on result to remove warning added by scan-build. r=mcmanus@ducksong.com
https://hg.mozilla.org/mozilla-central/rev/9f1dfef7a3e6
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: