MyBB Community Forums

Can we get this bug fixed sometime soon?
It's been there for a while, and the fix isn't that complex.

In attachment.php replace

echo file_get_contents($mybb->settings['uploadspath']."/".$attachment['attachname']);

with

$fp = fopen($mybb->settings['uploadspath']."/".$attachment['attachname'], "rb");
while(!feof($fp)) echo fread($fp, 65536);
fclose($fp);

Not exactly ideal, but is simple and fixes the problem.

---

Whilst I'm here, might I suggest that cache header handling be improved for attachments?
Currently the attachment script doesn't indicate that the attachment can be cached client side (although it does suppress sending no-cache headers). I haven't tried other browsers, but Firefox does always resend a request because of this, which the reply is always the full attachment.
Indicating the attachment can be cached client side would be helpful for reducing load/bandwidth with thumbnails and users wishing to reopen an attached image, for example. Or at least handle requests with a HTTP 304 response when necessary.

Example MyBB attachment download response headers:

HTTP/1.1 200 OK
Date: Sat, 29 Dec 2012 10:27:18 GMT
Server: Apache/2.4.2 (Win32) PHP/5.4.0
X-Powered-By: PHP/5.4.3
Content-disposition: inline; filename="image.jpg"
Content-Length: 85020
Content-range: bytes=0-85019/85020
Keep-Alive: timeout=3, max=8
Connection: Keep-Alive
Content-Type: image/jpeg

Example XThreads attachment response:

HTTP/1.1 200 OK
Date: Sat, 29 Dec 2012 10:27:52 GMT
Server: Apache/2.4.2 (Win32) PHP/5.4.0
X-Powered-By: PHP/5.4.3
Accept-Ranges: bytes
Allow: GET, HEAD
Last-Modified: Tue, 20 Nov 2012 09:35:29GMT
Expires: Sat, 05 Jan 2013 10:27:52GMT
Cache-Control: max-age=604800
ETag: "xthreads_attach_c1371fff_2_1353404129_7ddc33ff"
Vary: Range
Content-Disposition: inline; filename="image.jpg"
Content-Length: 85020
Content-Range: bytes 0-85019/85020
Content-MD5: fN8FQ4RnEqLWYWtmobS3yA==
Keep-Alive: timeout=3, max=8
Connection: Keep-Alive
Content-Type: image/jpeg

XThreads attachment response to an F5 (normal re-requests are browser cached, so don't trigger a request):

HTTP/1.1 304 Not Modified
Date: Sat, 29 Dec 2012 10:27:59 GMT
Server: Apache/2.4.2 (Win32) PHP/5.4.0
Connection: Keep-Alive
Keep-Alive: timeout=3, max=8
ETag: "xthreads_attach_c1371fff_2_1353404129_7ddc33ff"
Vary: Range

Thanks.

I agree.

Your code doesn't check if fopen succeeded and might turn into an endless loop if it failed (because the file went awol or whatever).

It may be safer and simpler to use readfile() instead of echo file_get_contents().

(2012-12-29, 01:12 PM)frostschutz Wrote: [ -> ]Your code doesn't check if fopen succeeded and might turn into an endless loop if it failed (because the file went awol or whatever).

It may be safer and simpler to use readfile() instead of echo file_get_contents().

Which is why the example isn't ideal.

Currently, if the file_get_contents call fails, it may output a warning which is sent as the download (or just send a blank file) - obviously not nice. Ideally you'd want to fopen before sending the Content-Disposition header so that you can output an error on failure without making browsers think that this is a valid download.
(alternatively, just consider that missing/unreadable attachments are a rare occurrence and leave it at that - I don't see this as a terribly big problem)

I wasn't aware of readfile (thanks to the huge list of functions PHP has) - thanks for pointing that out - but I am interested in how it handles failures. fopen can be checked before sending headers, but this doesn't appear to be possible with readfile.

It returns false if it couldn't read anything, and if it couldn't read anything it wouldn't have caused headers to be sent either, so you could still change the headers to 404 or handle errors any other way you like. But yes, with proper error handling, things are bound to get a tad more complicated.

Edit: oh, it does print an error message unless you suppress it with @. So you have to use @ if you want to do your own error handling, I guess - not sure if it changes the headers by itself, which would probably be weird.

readfile just returns false I believe (or it might output an error unless you use @ - I can't remember) if there's a problem. It's not exactly ideal as really you want to check if there is one before setting the headers (as really a 404 should be set if the attachment file does not exist). Maybe combining it with file_exists would help?

If you want to check before to avoid weird header mongering, use is_readable() instead of just file_exists(). That should catch most cases...

(2012-12-29, 01:28 PM)frostschutz Wrote: [ -> ]It returns false if it couldn't read anything, and if it couldn't read anything it wouldn't have caused headers to be sent either, so you could still change the headers to 404 or handle errors any other way you like. But yes, with proper error handling, things are bound to get a tad more complicated.

Edit: oh, it does print an error message unless you suppress it with @. So you have to use @ if you want to do your own error handling, I guess - not sure if it changes the headers by itself, which would probably be weird.

Documentation doesn't say that it somehow removes headers from output. And PHP doesn't seem to have a function to remove headers from output either, though you could send a HTTP 404 header after failure and pray that the previous header() calls haven't been flushed yet.

Your suggestion for is_readable() should be sufficient though - up to whoever implements it to choose from fopen or is_readable+readfile.

Seems pretty reasonable to me. Would love it if we had a Filesystem API to make things easier, but alas.

(2012-12-29, 01:41 PM)Yumi Wrote: [ -> ]And PHP doesn't seem to have a function to remove headers from output either

header_remove() works fine as long as headers haven't been sent already.