nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used

RESOLVED FIXED in Firefox 62

Status

()

defect
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

58 Branch
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

a year ago
NS_InputStreamIsBuffered() checks if the inputStream implements ReadSegments() correctly.
But if nsMultiplexInputStream contains more than 1 stream, and just the first one supports ReadSegments(), NS_InputStreamIsBuffered() will return true.

See more in bug 1436809.
Assignee

Updated

a year ago
Depends on: CVE-2018-5153
Assignee

Comment 1

a year ago
Posted patch multistream.patch (obsolete) — Splinter Review
Attachment #8974666 - Flags: review?(michal.novotny)
Comment on attachment 8974666 [details] [diff] [review]
multistream.patch

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

Thanks for the patch. Looks good, but you might need to have r+ from xpcom peer...
Attachment #8974666 - Flags: review?(michal.novotny) → review+
Assignee

Updated

a year ago
Attachment #8974666 - Flags: review?(nfroyd)
Comment on attachment 8974666 [details] [diff] [review]
multistream.patch

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

This review took way longer than it should have; my fault.

Just to make sure I understand the situation: comment 0 says that NS_InputStreamIsBuffered will only check the first stream for multiplex streams, is that correct?  And so if we add non-buffered streams to a multiplex stream after the first, then NS_InputStreamIsBuffered will return true, when it should be false?

Why are we not fixing this by fixing the NS_InputStreamIsBuffered check and avoiding forcing multiplex stream clients to always accept buffering?
Attachment #8974666 - Flags: review?(nfroyd)
Assignee

Comment 4

a year ago
See comment 18: https://bugzilla.mozilla.org/show_bug.cgi?id=1436809#c18

"This won't work when nsMultiplexInputStream is hidden behind some other stream. Let's say I'll land the ReadSegments implementation of SlicedInputStream, then we can have something like this:

sliced stream -> muxed stream -> ( string stream + file input stream )"

I also suggested to introduce a new interface and move ReadSegments() in that new interface, but that is a huge amount of work.
Assignee

Updated

a year ago
Flags: needinfo?(nfroyd)
And I suppose the sliced stream can't explicitly inform the muxed stream that it wants buffering on all of its substreams?  Or the sliced stream doesn't even know if the underlying stream is a muxed stream?  We could check that the sliced stream's substream is a muxed stream, and force buffering if so...but I guess that leads us to sprinkling checks for muxed streams everywhere, which is unattractive.  And if muxed streams can be created without buffering, and then find themselves requiring buffering at some later point, that creates its own set of issues.

Ugh.  It'd be so much nicer if NS_InputStreamIsBuffered just worked off straight interfaces...but maybe we'd still have this problem.

I think we may have to reluctantly accept buffering everywhere.  Can you please add a test to demonstrate that your patch will make ReadSegments (or buffering, whichever) work on multiplexed streams when you add a buffered stream and then an unbuffered stream?
Flags: needinfo?(nfroyd)
Assignee

Comment 6

a year ago
> And I suppose the sliced stream can't explicitly inform the muxed stream
> that it wants buffering on all of its substreams?  Or the sliced stream
> doesn't even know if the underlying stream is a muxed stream?  We could

Sliced don't know that the calling of ReadSegments() is executed in order to know if the stream is buffered.
Sliced (like many other 'meta' streams, such as buffered, mime, IPCBlob and so on) just forwards the request to the underlying stream.
Comment on attachment 8979441 [details] [diff] [review]
multistream_test.patch

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

WFM, thank you!
Attachment #8979441 - Flags: review?(nfroyd) → review+

Comment 9

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea17fb77684
nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used, r=michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc358abeba45
nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used - gtests, r=froydnj
Assignee

Comment 11

a year ago
Posted patch part 1Splinter Review
In order to make tests happy, GetStream() must return the original stream and not the buffered one.
Attachment #8974666 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8982007 - Flags: review?(nfroyd)
Comment on attachment 8982007 [details] [diff] [review]
part 1

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

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +254,5 @@
> +    nsCOMPtr<nsIInputStream> bufferedStream;
> +    nsresult rv = NS_NewBufferedInputStream(getter_AddRefs(bufferedStream),
> +                                            stream.forget(), 4096);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    stream = bufferedStream;

bufferedStream.forget()?
Attachment #8982007 - Flags: review?(nfroyd) → review+

Comment 13

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea106440d1b
nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used, r=michal, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/fae4d7e57052
nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used - gtests, r=froydnj

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ea106440d1b
https://hg.mozilla.org/mozilla-central/rev/fae4d7e57052
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Updated

10 months ago
Blocks: 1480354
Depends on: 1514581
You need to log in before you can comment on or make changes to this bug.