Closed Bug 1026761 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: erahm, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 749761])

Attachments

(1 file)

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
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Summary: nsAStreamCopier::Process can use sourceCondition, sinkCondition uninitialized if mCancel is true → CID 749761: nsAStreamCopier::Process can use sourceCondition, sinkCondition uninitialized if mCancel is true
Blocks: 508605
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.
Attachment #8616440 - Flags: review?(nfroyd)
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.
Attachment #8616440 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/89fff7bb133e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.