Closed Bug 1226526 Opened 4 years ago Closed 4 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)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
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)

The Static Analysis tool Scan-Build added a null pointer error in  GetIOService from netwerk/protocol/http/nsHttpHandler.cpp.
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.