1453015 - nsBufferedInputStream::Available could return wrong values after a aborted ReadSegments()

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Andrea Marchesini [:baku]]

https://searchfox.org/mozilla-central/source/netwerk/base/nsBufferedStreams.cpp#381-390

This code behaves wrongly if the underlying stream returns NS_BASE_STREAM_CLOSED but the internal buffer of nsBufferedInputStream contains some data. The correct behavior would be to return NS_OK until the buffer contains data.

In order to reproduce this issue, let's imagine this scenario:

1. a small inputStream, wrapped by a nsBufferedInputStream.
2. nsBufferedInputStream::ReadSegments() is called. The writing function returns an error.
3. nsBufferedInputStream::Available() is called.

in this scenario, point 2 reads the whole underlying inputStream. Any extra ::Available() call to the underlying stream returns NS_BASE_STREAM_CLOSED.
Because the writing function passed to nsBufferedInputStream::ReadSegments() aborts the operation, nsBufferedInputStream doesn't increment the mCursor value. nsBufferedInputStream::Available() fails.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: bufferedInputStream_available.patch

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8966637 [details] [diff] [review]
bufferedInputStream_available.patch

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

::: netwerk/base/nsBufferedStreams.cpp
@@ +403,5 @@
>          rv = Source()->Available(result);
> +        if (rv == NS_BASE_STREAM_CLOSED && remaining) {
> +            *result = remaining;
> +            return NS_OK;
> +        }

Good catch!

I think in case of ANY error (except probably WOULD_BLOCK) we should return {NS_OK, remaning}, because we have data to give. 

With this patch when the source stream is closed with some other error than STREAM_CLOSED, we will throw it, while we still actually (may) have data the consumer may freely read.

If you believe we have to throw errors other than STREAM_CLOSED (actually including WOULD_BLOCK) then feel free to oppose.

But I think this should look like:

avail = mFill - mCur;
rv = source()->avail(temp);
if (SUCC(rv)) avail += temp;
if (avail) { *result = avail; return NS_OK; }
return rv;

Comment 3

[Andrea Marchesini [:baku]]

Attachment: bufferedInputStream_available.patch

Your approach makes sense. Here the patch.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8967114 [details] [diff] [review]
bufferedInputStream_available.patch

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

thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1ce0dd15f1
nsBufferedInputStream::Available must return NS_OK until its buffer contains data, r=mayhemer

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7c1ce0dd15f1