NS_NewBufferedInputStream should take the ownership of the inputStream

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Posted patch fix2.patch (obsolete) — Splinter Review
As other wrapper, it's important that the bufferedInputStream takes the ownership of the input stream in order to avoid the use of it outside the wrapper.
Attachment #8919223 - Flags: review?(bugs)
Blocks: 1409329
Posted patch fix2.patchSplinter Review
Attachment #8919223 - Attachment is obsolete: true
Attachment #8919223 - Flags: review?(bugs)
Attachment #8919242 - Flags: review?(bugs)
Comment on attachment 8919242 [details] [diff] [review]
fix2.patch

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

::: netwerk/base/nsNetUtil.cpp
@@ +1370,5 @@
>  }
>  
>  MOZ_MUST_USE nsresult
> +NS_NewBufferedInputStream(nsIInputStream** aResult,
> +                          already_AddRefed<nsIInputStream> aInputStream,

Doesn't this mean if NS_NewBufferedInputStream() returns failure you lose the ref?  How about use nsCOMPtr<nsIInputStream>& and Move() it if you successfully take ownership.

Also, this is a pretty weak protection anyway.  Code can easily make nsCOMPtr<> on the stack to pass to this, etc.
> Doesn't this mean if NS_NewBufferedInputStream() returns failure you lose
> the ref?  How about use nsCOMPtr<nsIInputStream>& and Move() it if you
> successfully take ownership.

This fails only if we have allocation issue. And usually we are talking about 512, 4096 bytes. We can probably mark this method as infallible.

> Also, this is a pretty weak protection anyway.  Code can easily make
> nsCOMPtr<> on the stack to pass to this, etc.

Currently we don't have any better mechanism to protect bugs like what we had in FileReader where an inputStream was used via BufferedInputStream and outside it. This is an extra operation to do, and a reviewer can maybe catch this as a bug.
> This fails only if we have allocation issue. And usually we are talking
> about 512, 4096 bytes. We can probably mark this method as infallible.

Actually, 'new' doesn't fail: https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsBufferedStreams.cpp#67-68,77
Comment on attachment 8919242 [details] [diff] [review]
fix2.patch

>@@ -469,47 +469,48 @@ nsSAXXMLReader::ParseFromString(const ns
>   nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream),
>                                       data.get(), data.Length(),
>                                       NS_ASSIGNMENT_DEPEND);
>   NS_ENSURE_SUCCESS(rv, rv);
>   return ParseFromStream(stream, "UTF-8", aContentType);
> }
> 
> NS_IMETHODIMP
>-nsSAXXMLReader::ParseFromStream(nsIInputStream *aStream,
>+nsSAXXMLReader::ParseFromStream(nsIInputStream *aStreamPtr,
I don't understand this renaming. Ptr postfix is unusual.
I assume you did the change in order to guarantee other fixes are right, but could you undo this renaming
Attachment #8919242 - Flags: review?(bugs) → review+
Priority: -- → P2
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4666cde20909
NS_NewBufferedInputStream should take the ownership of the inputStream, r=smaug
Is there some stream that needs to have Close() called on it that isn't getting it any more since we're now clearing the reference in these patches?
And I'm sorry, but I pushed bug 1204254 which will conflict with your patches a bit.  (I removed one of the uses of this API.)
(In reply to Ben Kelly [:bkelly] from comment #9)
> Is there some stream that needs to have Close() called on it that isn't
> getting it any more since we're now clearing the reference in these patches?

If we do that, then this is a bug. Closing a BufferedInputStream means closing the original stream as well.
If we close the originalStream without closing the bufferedInputStream, this is definitely a bug.
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e2e65211f5
NS_NewBufferedInputStream should take the ownership of the inputStream, r=smaug
https://hg.mozilla.org/mozilla-central/rev/23e2e65211f5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.