Closed Bug 1005323 Opened 6 years ago Closed 5 years ago
Temporary File Input Stream::Read Segments will toss data written to non-blocking stream
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?
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...
- 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.
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 :-)
Target Milestone: --- → mozilla33
Attachment #8416917 - Flags: feedback?(jimb) → feedback+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.