1026761 - CID 749761: nsAStreamCopier::Process can use sourceCondition, sinkCondition uninitialized if mCancel is true

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

This is essentially the same issue as bug 504170, but in a different part of the function [1].

http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp?from=nsAStreamCopier::Process&case=true#355-356

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: initialize-sourceCondition-sinkCondition.patch

When we enter the for (;;) loop for the first time and `canceled` is true:

https://hg.mozilla.org/mozilla-central/file/d9b5c7f26d63/xpcom/io/nsStreamUtils.cpp#l322

and mCloseSink is non-null:

https://hg.mozilla.org/mozilla-central/file/d9b5c7f26d63/xpcom/io/nsStreamUtils.cpp#l368

and mAsyncSink is null:

https://hg.mozilla.org/mozilla-central/file/d9b5c7f26d63/xpcom/io/nsStreamUtils.cpp#l370

and we have an nsISafeOutputStream, then NS_SUCCEEDED() is called with uninitialized sourceCondition and sinkCondition:

https://hg.mozilla.org/mozilla-central/file/d9b5c7f26d63/xpcom/io/nsStreamUtils.cpp#l379

In this scenario where we enter Process() with an nsISafeOutputStream and `canceled` already true, do we want to call sostream->Finish() or just mSink->Close()? If we want to call sostream->Finish(), then the initial values of sourceCondition and sinkCondition should be some non-error result like NS_OK. If we only want to call mSink->Close(), then sourceCondition and sinkCondition should be some error result. In this patch, however, I initialize sourceCondition and sinkCondition with the mCancelStatus result, which is an error if `canceled` is true and NS_OK when `canceled` is false.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8616440 [details] [diff] [review]
initialize-sourceCondition-sinkCondition.patch

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

::: xpcom/io/nsStreamUtils.cpp
@@ +282,5 @@
>        cancelStatus = mCancelStatus;
>      }
>  
> +    nsresult sourceCondition = cancelStatus;
> +    nsresult sinkCondition = cancelStatus;

I think your reasoning makes sense; there should be a comment here describing why we are initializing things this way.  The key thing, I think, is why not calling Finish() on the safe output stream if we've been canceled is the right thing to do.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89fff7bb133e

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89fff7bb133e