Closed Bug 1005323 Opened 6 years ago Closed 5 years ago

nsTemporaryFileInputStream::ReadSegments will toss data written to non-blocking stream

Categories

(Core :: Audio/Video: Recording, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jimb, Assigned: jimb)

References

Details

Attachments

(1 file)

It is the convention for implementations of nsIInputStream::ReadSegments to not propagate errors returned by the writer function, but they should *not* consider the data passed to the failing writer call to have been transferred, nor attempt to write more data after that. They should return NS_OK and return an accurate write count.

However, nsTemporaryFileInputStream::ReadSegments ignores the count returned by the writer (other than to assert that it was not too much), and continues to call the writer function after it returns an error. This means that, for example, passing an nsTemporaryFileInputStream as the first argument to nsSocketOutputStream::WriteFrom, which calls the input stream's ReadSegments method, will simply throw away data if the socket is in non-blocking mode and returns NS_BASE_STREAM_WOULD_BLOCK.

I found this problem via an audit; while the code looks wrong I haven't actually observed it causing any real-world problems. How can I write an xpcshell test or mochitest that actually produces an nsTemporaryFileInputStream instance?
Flags: needinfo?(rlin)
It also seems odd that the return value from PR_Read is ignored; if it returns a partial count, we still ask writer to consume a full bufCount bytes.
The code also ignores the 'count' argument passed to ReadSegments.
(In reply to Jim Blandy :jimb from comment #2)
> The code also ignores the 'count' argument passed to ReadSegments.

That's incorrect. Sloppy reading on my part.
(In reply to Jim Blandy :jimb from comment #1)
> It also seems odd that the return value from PR_Read is ignored; if it
> returns a partial count, we still ask writer to consume a full bufCount
> bytes.

I see now that, since the count passed to PR_Read is limited to the amount known to be available in the file (as reflected in mStartPos and mEndPos), short reads shouldn't occur.
Hi Jim, 
Did you find something wrong about the EncodedBufferCache implementation?
I'm not sure the ni question...
Flags: needinfo?(rlin)
(In reply to Randy Lin [:rlin] from comment #5)
> Did you find something wrong about the EncodedBufferCache implementation?

I think so --- the needinfo was to ask for help in constructing a test case that might show the problem. Is there any way that a nsTemporaryFileInputStream might be visible to JavaScript? Because I believe it will fail if its ReadSegments method is used with a writer function that blocks.
- Respect the byte count that writer returns; don't assume it always accepts the full amount.

 - If writer returns an error, return NS_OK with a partial write count; don't continue writing data.

 - Simplify counters slightly.
Try push for above patch:
https://tbpl.mozilla.org/?tree=Try&rev=f05026c3b84c
It looks like nsTemporaryFileInputStream is not exposed to the JS side.
However, you can write a gtest to test the native components.
https://developer.mozilla.org/en-US/docs/GTest
You can request :roc help to review this one. thanks.
Duplicate of this bug: 1010084
Comment on attachment 8416917 [details] [diff] [review]
In nsTemporaryFileInputStream::ReadSegments, call writer correctly.

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

Don't forget this patch :-)
Attachment #8416917 - Flags: review+
Attachment #8416917 - Flags: feedback?(jimb)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4738365e0a1
Flags: in-testsuite-
Target Milestone: --- → mozilla33
Attachment #8416917 - Flags: feedback?(jimb) → feedback+
Assignee: nobody → jimb
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/e4738365e0a1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1333384
You need to log in before you can comment on or make changes to this bug.