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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fietserwin’s picture

Status: Active » Needs review
FileSize
689 bytes

This patch should solve the problem.

fietserwin’s picture

Writing a test is possible. So attached patch also includes a test.

Status: Needs review » Needs work

The last submitted patch, 1954180-2.patch, failed testing.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
1015 bytes

Changed the test, so that only 1 handle to the file is open.

Status: Needs review » Needs work

The last submitted patch, 1954180-3.patch, failed testing.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
3.05 KB
1.77 KB

That'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.

jthorson’s picture

I 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).

fietserwin’s picture

- 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.

jthorson’s picture

K ... looks good.

One final nitpick: If we're going to explicitly open up the 'b' mode, we should probably do the same for 't'.

Damien Tournoud’s picture

Status: Needs review » Needs work

I was going to say that, rt is also a valid mode.

jthorson’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
4.36 KB

This patch adds support for 'rt' as well.

No new test added, as the 't' flag is Windows-specific.

fietserwin’s picture

FileSize
6.26 KB

Thanks 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.

fietserwin’s picture

FileSize
19.35 KB

This patch comes from #12 but with the logic from #11 (in_array instead of a series of comparisons).

Status: Needs review » Needs work

The last submitted patch, 1267508-13.patch, failed testing.

jthorson’s picture

Status: Needs work » Needs review

#13: 1267508-13.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1267508-13.patch, failed testing.

jthorson’s picture

#13: Uploaded wrong file?

fietserwin’s picture

Status: Needs work » Needs review
FileSize
6.24 KB

Oops... wrong file indeed.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

looks good, has tests, lets get it in

ParisLiakos’s picture

#18: 1954180-13.patch queued for re-testing.

Pancho’s picture

Priority: Normal » Major

#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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1954180-13.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

#18: 1954180-13.patch queued for re-testing.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9dbbb4d and pushed to 8.x. Thanks!

Pancho’s picture

Awesome!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.