Make WordPress Core

Opened 14 years ago

Closed 8 years ago

Last modified 8 years ago

#14730 closed defect (bug) (fixed)

ms-files.php required ob_clean() and flush()

Reported by: pj_mfc's profile pj_mfc Owned by: jeremyfelt's profile jeremyfelt
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.0
Component: Multisite Keywords: has-patch
Focuses: multisite Cc:

Description

I recently migrated a development wp 3 multisite install to 'live' mode for a site launch. One problem I had is that images served out of /files stopped working. Everything looked fine as far as the response / headers, but the image would open up as corrupted.

After quite a bit of troubleshooting, the problem went away when I added ob_clean() and flush() calls to ms-files.php just before the readfile($file) call at the end of the file. Perhaps this is due to a problem elsewhere in my site, but I'm creating this bug so someone can decide if adding the buffer flush would be a reasonable addition to the codebase. It seems like this is the recommended way of doing things according to the sample code here:

http://us2.php.net/manual/en/function.readfile.php

Attachments (2)

ms-files.php.patch (185 bytes) - added by fuscata 14 years ago.
14730.diff (576 bytes) - added by iamfriendly 9 years ago.
actions, rather than ~words~ flushing.

Download all attachments as: .zip

Change History (38)

#1 @nacin
14 years ago

  • Component changed from General to Multisite

You most likely have something else going on that is causing a few bytes to slip into the output of the page. I'm not sure of the purpose is for ob_clean() in that example other than to prevent unrelated output buffers from interfering. It may be a technique I simply haven't heard of before, but by default ms-files.php shouldn't cause those kinds of problems.

#2 @mikelittle
14 years ago

  • Cc wordpress@… added

I encountered a similar problem on two different installations of WordPress when upgrading from wpmu 2.9.2 to wp 3.0.1.

This is a long entry, but hopefully has plenty of concrete information.

On the first server:

  • Ubuntu 8.04.4 LTS
  • PHP Version 5.2.4-2ubuntu5.10
  • Apache/2.2.8 (Ubuntu) DAV/2 SVN/1.4.6 PHP/5.2.4-2ubuntu5.10 with Suhosin-Patch proxy_html/3.0.0

I first noticed ms-files.php cutting off an image at 16k. Here is the output of a wget:

wget http://ias.version-control.com/files/2010/01/DarthVader.png
--2010-09-08 10:00:42--  http://ias.version-control.com/files/2010/01/DarthVader.png
Resolving ias.version-control.com... 87.117.205.87
Connecting to ias.version-control.com|87.117.205.87|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 16631 (16K) [image/png]
Saving to: `DarthVader.png'
98% [=...==>  ] 16,384      --.-K/s   in 0.04s   
2010-09-08 10:00:42 (365 KB/s) - Connection closed at byte 16384. Retrying.

Adding a call to flush() after readfile($file) solved the problem.

wget http://ias.version-control.com/files/2010/01/DarthVader.png
--2010-09-08 10:01:50--  http://ias.version-control.com/files/2010/01/DarthVader.png
Resolving ias.version-control.com... 87.117.205.87
Connecting to ias.version-control.com|87.117.205.87|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 16631 (16K) [image/png]
Saving to: `DarthVader.png.1'
100%[=...==>] 16,631      --.-K/s   in 0.05s   
2010-09-08 10:01:50 (343 KB/s) - `DarthVader.png.1' saved [16631/16631]

I then noticed the problem (or rather the client did) on the live site (which had the call to flush in place) on a larger file.

On the live server:

  • Red Hat Enterprise Linux Server release 5.4 (Tikanga)
  • PHP Version 5.1.6
  • Apache/2.2.3 (Red Hat)

ms-files.php was cutting off at 1097728 bytes. Here is the output of a wget:

wget http://imascientist.org.uk/files/2010/01/Im-a-Scientist-evaluation-report.pdf
--2010-09-07 16:38:34--  http://imascientist.org.uk/files/2010/01/Im-a-Scientist-evaluation-report.pdf
Resolving imascientist.org.uk... 94.236.120.219
Connecting to imascientist.org.uk|94.236.120.219|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 1099885 (1.0M) [application/pdf]
Saving to: `Im-a-Scientist-evaluation-report.pdf.4'
99% [=...===> ] 1,097,728   1.06M/s   in 1.0s    
2010-09-07 16:38:36 (1.06 MB/s) - Connection closed at byte 1097728. Retrying.

I went back to the first server and tried the larger file. Same result without the flush() in place. Here is the output of a wget:

wget http://ias.version-control.com/files/2010/01/Im-a-Scientist-evaluation-report.pdf
--2010-09-08 09:53:23--  http://ias.version-control.com/files/2010/01/Im-a-Scientist-evaluation-report.pdf
Resolving ias.version-control.com... 87.117.205.87
Connecting to ias.version-control.com|87.117.205.87|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 1099885 (1.0M) [application/pdf]
Saving to: `Im-a-Scientist-evaluation-report.pdf'
99% [==...==> ] 1,097,728    633K/s   in 1.7s    
2010-09-08 09:53:25 (633 KB/s) - Connection closed at byte 1097728. Retrying.

With the flush() in place the large file also downloads successfully.

Back on the live server, I spotted that even small files do not download with the flush in place: here is wget again for a much smaller file:

wget http://imascientist.org.uk/files/2010/04/ias-ticket-wallet-18th-june-09-thumbnail1-150x117.jpg
--2010-09-08 10:49:24--  http://imascientist.org.uk/files/2010/04/ias-ticket-wallet-18th-june-09-thumbnail1-150x117.jpg
Resolving imascientist.org.uk... 94.236.120.219
Connecting to imascientist.org.uk|94.236.120.219|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 9297 (9.1K) [image/jpeg]
Saving to: `ias-ticket-wallet-18th-june-09-thumbnail1-150x117.jpg.1'
88% [=...==>             ] 8,192       --.-K/s   in 0.009s  
2010-09-08 10:49:24 (925 KB/s) - Connection closed at byte 8192. Retrying.

Here it is cutting off at 8k rather than 16k

This time after much googling, I found a replacement readfile here http://us2.php.net/manual/en/function.readfile.php#88549

Adding that function to ms-files.php and calling that in place of readfile solves the problem on the live site: both small and large files are downloaded ok.

In both cases the problem did not occur with WordPress MU 2.9.2.

In neither case has the OS, PHP, or Apache changed. They are both dedicated servers and I have full control.

I noticed that for both the large and small files, the cut off point was an 'exact' size (16K and 67 x 16K for server 1, 8K and 134 x 8K for the live server), so this is probably an interaction with a PHP buffer somewhere.

Summary:

ms-files.php call to readfile() seems to be not flushing it's last buffer for both large and small files.

On the test server, a call to flush() solves the problem for both large and small files.

On the live server, the call to flush was not enough for small or large. The replacement readfile function was required.

The servers have different OS, different version of PHP and different versions of Apache, so this is not a simple "bug in version x of y" issue.

The difference between blogs.php from mu 2.9.2 and ms-files.php in wp 3.0.1 is minimal, but something in the 3.0.1 environment is triggering this issue.

I hope this helps someone get to the heart of the issue.

#3 @westi
14 years ago

  • Owner set to westi
  • Status changed from new to accepted

#4 @mikelittle
14 years ago

Just encountered another issue on the live server, where mod_deflate was interfering too. Uploaded file (/files/) files would not download to a browser with mod_deflate enabled. I didn't notice it with wget as it doesn't support deflate.

It should have only been turned on for certain files types

AddOutputFilterByType DEFLATE text/html text/css application/x-javascript text/plain text/xml

but seemed to be also applied to files served via ms-files.php

I've turned it off for now as I couldn't figure out how to turn it off just for those files. That is Apache related, but maybe the issue is with an Apache buffer not PHPs.

By the way, having turned off mod_deflate, I quickly rolled back to normal readfile in ms-files.php and the problem re-appeared.

Westi, if you need access to my test server I could give you that.

#5 @pj_mfc
14 years ago

I just ran into the same issue on a production wp3 multisite installation after the latest wp upgrade wiped out my hard-coded changes. My sites are running on dreamhost and my install is quite typical. I've run plenty of php (Wordpress, Joomla, and custom sites) on this host and never encountered any similar problems. I'm happy to help debug this if anyone is interested in following up. As stated in my original post, it looks like php.net is recommending using the buffer flushing functions in combination with readfile(). The comments on the page linked above confirm that there are known issues using readfile() without flushing the output buffer beforehand, so it seems like the proper thing to do here is update ms-files.php in wordpress.

#6 @SergeyBiryukov
14 years ago

Unexpected output can be caused by UTF-8 byte order mark or spaces before <?php or after ?> in wp-config.php, theme's functions.php or any of the plugins. In this case there might be other problems than serving files with ms-files.php.

#7 @MikeLittle
14 years ago

Sergey, I think you have the wrong idea about this issue; it is not that the output is corrupted by something at the beginning, but that it is truncated -- the end is missing.

#8 @SergeyBiryukov
14 years ago

It seems to me that you and pj_mfc actually have two different issues. Am I wrong?

#9 @nacin
14 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#10 @MikeLittle
14 years ago

@Sergy, I don't think so.

At least, if, as pj_mfc states, the flush() after readfile() fixed the problem, then the problem is not extra data output before an image file, corrupting it: It would still be corrupted with the flush().

#11 @fuscata
14 years ago

@MikeLittle, I think Sergy is correct. You (assuming mikelittle == MikeLittle) are talking about putting a flush() after the readfile() call, while pj_mfc is talking about putting ob_clean() and flush() before the readfile() call.

I came here to report pj_mfc's problem/solution myself, so can confirm both (the problem and the fix).

As described here: http://core.trac.wordpress.org/ticket/16306 there's not much that can be done about errant whitespace in theme/plugin files, so WP should clean & flush before the readfile() call.

And, heck, let's flush() again, as you suggested, in case anything is left in the bowl, er, buffer.

#12 @fuscata
14 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#13 @MikeLittle
14 years ago

@fuscata OK. I'm happy to accept that whitespace output before ms-files.php gets to do it's thing will match the symptoms originally reported here.

I may have inadvertently coloured my reading of it with my own symptoms, but my case was definitely not that situation, and nor was it solved in all cases by a simple flush() after the call to readfile().

When I get a chance (likely after this week) I will try reproducing the problem again with 3.0.4 (i.e taking out my custom code)and if I *can* reproduce it will raise a new ticket for that problem.

#14 @willmot
13 years ago

I reproduced this on WordPress 3.2.1 although it only affected files that were smaller than 8k.

Any image larger than 8k works fine, images smaller than 8k return nothing

$ wget http://triblocal.dev/evanston/files/cache/2012/01/Selver_3/786845602.jpg
--2012-01-05 14:56:26--  http://triblocal.dev/evanston/files/cache/2012/01/Selver_3/786845602.jpg
Resolving triblocal.dev (triblocal.dev)... 127.0.0.1
Connecting to triblocal.dev (triblocal.dev)|127.0.0.1|:80... connected.
HTTP request sent, awaiting response... No data received.
Retrying.

After I add flush(); after the readfile(); it works

$ wget http://triblocal.dev/evanston/files/cache/2012/01/Selver_3/786845602.jpg
--2012-01-05 14:58:11--  http://triblocal.dev/evanston/files/cache/2012/01/Selver_3/786845602.jpg
Resolving triblocal.dev (triblocal.dev)... 127.0.0.1
Connecting to triblocal.dev (triblocal.dev)|127.0.0.1|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 7306 (7.1K) [image/jpeg]
Saving to: `786845602.jpg.1'

100%[====================================================================================================================================================================================================================================================================================>] 7,306       --.-K/s   in 0s      

2012-01-05 14:58:11 (332 MB/s) - `786845602.jpg.1' saved [7306/7306]
Last edited 13 years ago by willmot (previous) (diff)

#15 @kurtpayne
13 years ago

  • Cc kpayne@… added

The following info will help other devs to diagnose and reproduce the problem and test the patch:

  • What OS is your server using?
  • What web server are you using?
  • With or without gzip / deflate compression?
  • Are you using mod_php or a cgi variant?
  • What is your server's php version?
  • With ob_gzhandler enabled or zlib.output_compression enabled?
  • Are you going through a proxy (or have a reverse proxy in front of your web servers)?
  • Can you reproduce this different browsers, or just wget?
  • Have you tried different file types or just images?

By the way, I had a good discussion with dd32 in #18525 about buffering / caching related to flush() that may be in some way related.

#16 @Ipstenu
13 years ago

  • Cc ipstenu@… added

#17 @dragunoff
12 years ago

  • Cc dragunoff@… added

#18 @jeremyfelt
12 years ago

  • Keywords close added

Now that ms-files.php is no longer relied on for new installs, this issue is probably not a priority. Suggesting we close as wontfix unless we think there's a clear reason to move forward.

#19 @jeremyfelt
11 years ago

  • Keywords needs-testing close removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from accepted to closed

The original issue reported sounds more like an issue where characters were being output unknowingly before readfile() occured.

#20 @nacin
11 years ago

  • Resolution changed from wontfix to worksforme

If anyone does continue to run into this issue in 3.6 or later, I am OK with adding a flush() after readfile() as that seems harmless, if that indeed fixes said problem. Please reopen if so.

Changing to worksforme as this is an issue of not knowing what the problem is or how to fix it; versus wontfix which would indicate we are deliberately not fixing it.

#21 @stefwilliams
10 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened
  • Version set to 4.2.2

We have a multisite install where this issue keeps recurring (currently on 4.2.2).
A number of times after an update, we've had broken images, and have had to edit ms-files.php in order to fix it.
As far as I can tell, this is ongoing, and if adding the flush call is indeed harmless, it would certainly help.
There also appear to be a few support threads dotted around reporting similar issues with no clear fix. Who knows, this might resolve it for them..?

For the record, we just upgraded to 4.3 and had to edit ms-files.php again

Version 1, edited 9 years ago by stefwilliams (previous) (next) (diff)

#22 @dd32
10 years ago

  • Milestone set to Awaiting Review

#23 @jeremyfelt
9 years ago

  • Focuses multisite added

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


9 years ago

#25 @jeremyfelt
9 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Owner changed from westi to iamfriendly
  • Status changed from reopened to assigned

@iamfriendly is going to take a look at this with a large multisite install. :)

Let's do what we can to either patch or close in 4.6.

This ticket was mentioned in Slack in #core-multisite by richardtape. View the logs.


9 years ago

#27 follow-up: @iamfriendly
9 years ago

Replicating this delightful issue isn’t easy. It doesn’t appear to happen on every upgrade. Nailing down exactly why it happens appears to be beyond me.

However, the patch — as-is — doesn’t ‘break’ anything as far as I can tell. It seems harmless. After applying it, and running several upgrades, I haven’t been able to replicate the broken images. :tada:

That being said, I’m starting to think that an even safer strategy is to add actions before and after the readfile() rather than some magic flushing. This way, the change would be completely benign on our part, and if the flushing/cleaning is the answer to all life’s problems for some installs, then it can be added in a mu-plugin for those affected. Thoughts?

@iamfriendly
9 years ago

actions, rather than ~words~ flushing.

#28 in reply to: ↑ 27 @jeremyfelt
9 years ago

Replying to iamfriendly:

That being said, I’m starting to think that an even safer strategy is to add actions before and after the readfile() rather than some magic flushing.

This would be great, but because of the SHORTINIT in ms-files.php, not even mu-plugins are available. :/

#29 @iamfriendly
9 years ago

Darn.

OK, in that case, my initial comment stands. The magic flushing certainly didn't break anything on our environment in any of the test updates I ran. I heavily suspect that adding them will not be harmful to other folks still using ms-files.php - perhaps it's worthy of going in relatively early in the cycle to allow people to test.

#30 @jeremyfelt
9 years ago

  • Keywords 4.7-early added
  • Milestone changed from 4.6 to Future Release

I'm going to bump this. I'd like to understand exactly what it is we're solving first and get it in early. Let's do 4.7 instead.

This ticket was mentioned in Slack in #core by helen. View the logs.


8 years ago

#32 @jeremyfelt
8 years ago

  • Keywords 4.7-early removed
  • Milestone changed from Future Release to 4.7
  • Owner changed from iamfriendly to jeremyfelt
  • Status changed from assigned to reviewing

#33 @jorbin
8 years ago

@jeremyfelt What is needed to help prevent this from being bumped from 4.7? Can you lay out an attack plan?

#34 @jeremyfelt
8 years ago

I'm going to go ahead and commit part of the original patch and place a flush() after readfile(). Based on @iamfriendly and my testing, I don't believe this will cause any issues. Based on the extensive testing in from several years ago, it seems like this is the right approach for something that appears to be incredibly hard to reproduce. :)

Notes on approaches:

  • The replacement readfile() function no longer exists at php.net, but can be found around the web as readfile_chunked() (not sure what its licensing is). It uses fopen(), fgets(), fread(), etc... to chunk through a larger file.
  • I can't make sense of adding ob_clean() and flush() before readfile(). There are no themes or plugins in play here, and only headers have been built by ms-files.php when readfile() is called. It's very possible I'm missing something, but I'd rather not add more than we need.
  • Looking at the source of readfile(), it (via php_stream_passthru) outputs all remaining data from stream to the active output buffer and returns the number of bytes output. If buffering is disabled, the data is written straight to the output. It seems harmless to then use flush() after to ensure all of the output has been pushed.

I'm still not confident in the "why", but this should resolve the issue. We can readdress if it continues to appear.

#35 @jeremyfelt
8 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 38662:

Multsite: Flush output buffer after readfile() in ms-files.php.

Props fuscata, MikeLittle for testing, iamfriendly for testing.
Fixes #14730.

#36 @jeremyfelt
8 years ago

  • Version changed from 4.2.2 to 3.0
Note: See TracTickets for help on using tickets.