1349903 - HttpChannelParent::DoAsyncOpen() would crash when a void LoadInfo is received

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Henry Chang [:hchang]]

Since |LoadInfoArgsToLoadInfo| might return NS_OK with a null output loadInfo [1], the |loadInfo| access below [2] needs a nullity check.

[1] http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/ipc/glue/BackgroundUtils.cpp#359
[2] http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/netwerk/protocol/http/HttpChannelParent.cpp#365

Comment 1

[Daniel Stenberg [:bagder]]

Tim, is looks like you did the HttpChannelParent.cpp change, you up to addressing this concern?

Comment 2

[Henry Chang [:hchang]]

I am also aware the originAttributes we get from loadInfo is not used at all.
That part can be removed. The rest of concern is when we get a null loadInfo,
should we return error or pass it around. (loadInfo will be used to new the
actual channel a couple of lines below.)

Comment 3

[Tim Huang[:timhuang]]

The original purpose of the originAttributes here is for appId. However, the appId is gone. So I think we can remove this part of code. And I think we should return an error when we get a null loadInfo since every channel should have a loadInfo.

Comment 4

[Tim Huang[:timhuang]]

Attachment: Bug 1349903 - Remove the code section of getting originAttributes from loadInfo in HttpChannelParent::DoAsyncOpen() and add a null check for loadInfo.

The code section of getting originAttributes from loadInfo in HttpChannelParent
::DoAsyncOpen() is no longer needed since AppId is gone and the originAttributes
here is for AppId.

And we also need a null check for loadInfo that every channel should have a
loadInfo.

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

Comment 5

[Patrick McManus [:mcmanus]]

originAttributes is heavily used by tor for segmenting things by first party.. does that apply here?

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8851161 [details]
Bug 1349903 - Remove the code section of getting originAttributes from loadInfo in HttpChannelParent::DoAsyncOpen() and add a null check for loadInfo.

https://reviewboard.mozilla.org/r/123542/#review126264

has there been a test case for this?  seems like this has been reported w/o a crash report based only on code inspection

Comment 7

[Henry Chang [:hchang]]

I spotted this issue when I was initiating the IPDL-based fuzzing work.

As a starter/exploration/experiment, I picked some typical protocols to handicraft
their fuzzer. Compared to low level IPC fuzzing, the intention of IPDL fuzzing is to
be able to assemble more parameter-type-aware IPC message so that we can try to
attack the "handler" level instead of merely the IPC infrastructure.

Since PHttpChannel is so frequently used and is managed by PNecko/PContent (which
is the most fundamental parent protocols), it's chosen as my very first fuzzer.

Comment 8

[Tim Huang[:timhuang]]

Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80fd0d993f2e

Comment 9

[Carsten Book [:Tomcat]]

need final review/ship it on mozreview so that we can use the autoland feature.

Comment 11

[Tim Huang[:timhuang]]

Attachment: Remove the code section of getting originAttributes from loadInfo in HttpChannelParent::DoAsyncOpen() and add a null check for loadInfo. r?mayhemer

Carrying r+ from Honza. Please land this patch instead of the patch from
mozreivew. Thanks a lot.

Comment 12

[Tim Huang[:timhuang]]

Sheriff, could you please land the patch from comment 11 to inbound. Thanks a lot.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef7c269687e
Remove the code section of getting originAttributes from loadInfo in HttpChannelParent::DoAsyncOpen() and add a null check for loadInfo. r=mayhemer

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bef7c269687e