The LocalReadOnlyStream stream wrapper checks that the mode that get passed in with fopen is not trying to open the file in a non read only mode. However, it refuses the 'rb' mode, that is a valid mode for a read-only stream. file_transfer() (in core/includes/file.inc) opens a file with mode 'rb' and thus will fail if it gets passed in a streamwrapper that derives form LocalReadOnlyStream.
Writing a test to show this error is difficult as LocalReadOnlyStream is abstract and there are no actual uses of it yet. I found the error in a D7 install that has a module:\\ stream wrapper in a contrib module.
Comment | File | Size | Author |
---|---|---|---|
#18 | 1954180-13.patch | 6.24 KB | fietserwin |
#13 | 1267508-13.patch | 19.35 KB | fietserwin |
#12 | 1954180-11.patch | 6.26 KB | fietserwin |
#11 | 1954180-11.patch | 4.36 KB | jthorson |
#11 | interdiff.txt | 1.4 KB | jthorson |
Comments
Comment #1
fietserwinThis patch should solve the problem.
Comment #2
fietserwinWriting a test is possible. So attached patch also includes a test.
Comment #4
fietserwinChanged the test, so that only 1 handle to the file is open.
Comment #6
fietserwinThat's another error in LocalReadOnlyStream::stream_flush(): stream_flush() gets called during stream_close(). So stream_flush() should not trigger an error but fail silently.
And thus also an error in an existing test:
the test on fclose should not use @fclose as fclose is not expected to trigger warnings.
Attached are patches with corrected tests and code that removes both errors in LocalReadOnlyStream.
Comment #7
jthorson CreditAttribution: jthorson commentedI think stream_flush should trigger an error if it is called and there is anything in the output buffer; as that would indicate an attempt to 'write' to the file. Removing the error because fclose() calls it doesn't seem 100% correct; as an attempt to flush actual output to a file that was supposed to be read-only should trip the error. (That said, if PHP checks the read/write mode before allowing that data into the output buffer in the first place, and trips an error there, then the above argument is moot.)
Is there some way to check whether there is actual data being flushed, and using that to trip an error (or not)?
If we're simply removing the error as per this patch, I suspect that we can probably remove the entire method ... as FALSE is assumed if the method is not implemented by a given stream wrapper (unless we've defined it in one of the parent classes ... I don't recall).
Comment #8
fietserwin- LocalReadOnlyStream extends LocalStream which does implement stream_flush by returning
return fflush($this->handle);
. So it can't be removed.- I did a small test: calling fclose() on a stream wrapper will first execute stream_flush() and only then stream_close(). In other words: the stream_flush is not executed by the
fclose($this->handle)
in the stream_close() implementation. So setting a flag in stream_close to ignore the upcoming stream_flush won't work.- AFAIK there's no way to find out if there's something in the buffer. A bit of reading through the manual and Googling did not reveal anything that suggests that this can be found out.
So I think that leaves us no other option than to silently ignore calls to stream_flush. But as stream_write() calls are intercepted as well as any attempt to open a file other then in read-only mode, I don't think this is a problem.
By looking at the inheritance tree, I found out that ReadOnlyStream does not share its implementation with LocalReadOnlyStream and thus contains the same errors. The new patches also solve the same errors in ReadOnlyStream.
Comment #9
jthorson CreditAttribution: jthorson commentedK ... looks good.
One final nitpick: If we're going to explicitly open up the 'b' mode, we should probably do the same for 't'.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedI was going to say that,
rt
is also a valid mode.Comment #11
jthorson CreditAttribution: jthorson commentedThis patch adds support for 'rt' as well.
No new test added, as the 't' flag is Windows-specific.
Comment #12
fietserwinThanks for mentioning. Changes in the attached patch:
- Accept 'rt' as mode.
- Updated method documentation as it explicitly mentioned 'r' as only acceptable mode.
- Changed the test for mode 'w' to test 'r+' instead (might someone ever try to optimize the logic...).
- Added a test for mode 'rt'
I left out the test only patch as it should be clear by now that there is a bug in the code and that we have tests that can detect it.
EDIT: cross posted, I did add that test (mentioned in #11 and have tested it on a Windows machine, Let's see if it fails on non-Windows.
Comment #13
fietserwinThis patch comes from #12 but with the logic from #11 (in_array instead of a series of comparisons).
Comment #15
jthorson CreditAttribution: jthorson commented#13: 1267508-13.patch queued for re-testing.
Comment #17
jthorson CreditAttribution: jthorson commented#13: Uploaded wrong file?
Comment #18
fietserwinOops... wrong file indeed.
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedlooks good, has tests, lets get it in
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commented#18: 1954180-13.patch queued for re-testing.
Comment #21
Pancho#1308152: Add stream wrappers to access extension files depends on this, so raising priority. That one also brings an actual implementation with more tests, so we should be on the safe side with this one here.
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commented#18: 1954180-13.patch queued for re-testing.
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedback to rtbc
Comment #25
alexpottCommitted 9dbbb4d and pushed to 8.x. Thanks!
Comment #26
PanchoAwesome!
Comment #27.0
(not verified) CreditAttribution: commentedUpdated issue summary.