Unsafe use of mResponseHead in HttpBaseChannel::GetEntityID

RESOLVED FIXED in mozilla8

Status

()

Core
Networking: HTTP
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jdm, Assigned: Evan Shaw)

Tracking

unspecified
mozilla8
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=jdm][inbound])

Attachments

(1 attachment, 1 obsolete attachment)

1.80 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Either mResponseHead can be null in GetEntityID, or it can't, but these bits need to change one way or another:

>1203   const char* acceptRanges =
>1204       mResponseHead->PeekHeader(nsHttp::Accept_Ranges);
...
>1212   if (mResponseHead) {
>1213     size = mResponseHead->TotalEntitySize();
(Reporter)

Comment 1

6 years ago
This is a very simple bug to resolve. Whoever ends up working on it should get input from Jason or Honza on whether to add a null check or remove the existing one.
Whiteboard: [good first bug][mentor=jdm]
After on OnStartRequest we have to return something, nevertheless we have the response head.

The change with acceptRanges has been added in bug 462707 [1].  Ehsan apparently forgot to add a null check, I would say.

We should move his code into the if block.

[1] http://hg.mozilla.org/mozilla-central/rev/356e3865c629
(Assignee)

Comment 3

6 years ago
Created attachment 547345 [details] [diff] [review]
Proposed patch

I've chosen this bug for my first patch. Hopefully I got it right. :)
Attachment #547345 - Flags: review?(josh)
(Reporter)

Comment 4

6 years ago
Comment on attachment 547345 [details] [diff] [review]
Proposed patch

Thanks very much for your patch, Evan! It looks right to me, but I will defer to Honza's review on this, since he is a peer of this code. However, if you could upload another version that follows the guidelines at https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f then it will be easier to check in this patch once the review process is finished.
Attachment #547345 - Flags: review?(josh) → review?(honzab.moz)
(Assignee)

Comment 5

6 years ago
Created attachment 547613 [details] [diff] [review]
Patch 2

Patch 2 attached; hopefully in the correct format this time.
Attachment #547345 - Attachment is obsolete: true
Attachment #547613 - Flags: review?(honzab.moz)
Attachment #547345 - Flags: review?(honzab.moz)
Comment on attachment 547613 [details] [diff] [review]
Patch 2

Review of attachment 547613 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Attachment #547613 - Flags: review?(honzab.moz) → review+
(Reporter)

Comment 7

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/9dda487bc910

Thanks Evan!
Assignee: nobody → edsrzf
Whiteboard: [good first bug][mentor=jdm] → [good first bug][mentor=jdm][inbound]
http://hg.mozilla.org/mozilla-central/rev/9dda487bc910
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.