All users were logged out of Bugzilla on October 13th, 2018

nsSyncLoadService::PushSyncStreamToListener should not fail for channels of unknown size

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mark.yen, Assigned: mark.yen)

Tracking

Trunk
mozilla25
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 776658 [details] [diff] [review]
Check for chunkSize < 1

See attached patch.

nsIChannel's documentation says that, when a channel doesn't know its content length, .contentLength may be -1.  nsSyncLoadService::PushSyncStreamToListener instead keeps it at -1, then casts it to uint32_t (in the call to NS_NewBufferedInputStream).  This causes the buffered input stream to attempt to (fallibly) allocate a 4GB buffer.  In my case this leads to an out of memory error, which means the thing fails to load.

The trivial patch attached checks for when chunkSize < 1 and sets it to the default (4k) instead.

I'm not familiar with the testing around this area - if it's necessary (which means constructing a channel of unknown length, somehow), please point to the nearby tests so I can copy things off them :)

(I'm picking reviewer based on previous commits, and component more-or-less randomly... Please redirect as necessary, thanks!)
Attachment #776658 - Flags: review?(ehsan)

Updated

5 years ago
Attachment #776658 - Flags: review?(ehsan) → review?(jst)
Comment on attachment 776658 [details] [diff] [review]
Check for chunkSize < 1

This looks reasonable to me, but I'd love for someone in the networking team to sign off on this as well. jduell, can you either have a look or redirect as needed? Thanks!
Attachment #776658 - Flags: review?(jst)
Attachment #776658 - Flags: review?(jduell.mcbugs)
Attachment #776658 - Flags: review+
Comment on attachment 776658 [details] [diff] [review]
Check for chunkSize < 1

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

This looks fine to me.  So fine I might even be ok w/o a test.  But it would be nicer to have one :)

IIRC httpd.js adds a content-length header by default--I think that happens here:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js#3964

In any case I'm fairly certain you can omit having a Content-Length in the response if you SeizePower!

  http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_duplicate_headers.js#61

(yes, I wrote that comment)
Attachment #776658 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 778195 [details] [diff] [review]
Now with a test

I can't help but think that this test is a horror in itself and we're better of not having it.  Seriously, WTF.
Attachment #776658 - Attachment is obsolete: true
Attachment #778195 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 4

5 years ago
Oh, I guess I should write more about that test:
- HTTP channels actually implement readSegments, so this code path isn't used at all.
- The only useful thing around I could find that *doesn't* implement readSegments was the file stream stuff, because it knows better
- Of course, file channels know how long they are, so... I implement a nsIChannel so I can say I don't :|
- The rest is all fluff needed to get to that point.

Incidentally, this matches better why we're seeing this bug... a JS-implemented channel that reads (eventually, after some other stuff) from a file stream.
Comment on attachment 778195 [details] [diff] [review]
Now with a test

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

> I can't help but think that this test is a horror in itself 

You underestimate your very creative patch.  A work of art in its own, special way :)
Attachment #778195 - Flags: review?(jduell.mcbugs) → review+
Correction, Windows and Linux32 only.

Comment 9

5 years ago
Created attachment 782593 [details] [diff] [review]
fixed test

Sorry, difference between unpackaged and packaged tests :(
Fixed, now with a linux32 try run:

https://tbpl.mozilla.org/?tree=Try&rev=c500d6501af2
Attachment #778195 - Attachment is obsolete: true
Attachment #782593 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 10

5 years ago
Try 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc4bafd92a3
This time with a gree try run!
https://hg.mozilla.org/mozilla-central/rev/3bc4bafd92a3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.