News:

Wondering if this will always be free?  See why free is better.

Main Menu

Trying to access array offset on value of type bool

Started by sah62, July 23, 2022, 07:59:36 AM

Previous topic - Next topic

sah62

I just noticed this error in my error log:

QuoteTrying to access array offset on value of type bool

This is an installation of SMF 2.1.2, default theme, UTF-8 supported, no non-English language packs, level 1 caching, no mods, PHP 7.4.3.

Backtrace:

#0: smf_error_handler()
Called from /var/www/mysite/smf/Sources/Subs-Attachments.php on line 1251
#1: loadAttachmentContext()
Called from /var/www/mysite/smf/Sources/Subs-Attachments.php on line 1048
#2: parseAttachBBC()
Called from /var/www/mysite/smf/Sources/Subs.php on line 1746
#3: {closure}()
Called from /var/www/mysite/smf/Sources/Subs.php on line 3636
#4: parse_bbc()
Called from /var/www/mysite/smf/Sources/Display.php on line 1485
#5: prepareDisplayContext()
Called from /var/www/mysite/smf/Themes/default/Display.template.php on line 256
#6: template_main()
Called from /var/www/mysite/smf/Sources/Load.php on line 2747
#7: loadSubTemplate()
Called from /var/www/mysite/smf/Sources/Subs.php on line 4155
#8: obExit()
Called from /var/www/mysite/smf/index.php on line 194

The code involved:

1245: // Calculate the size of the created thumbnail.
1246: $size = @getimagesize($filename . '_thumb');
1247: list ($attachment['thumb_width'], $attachment['thumb_height']) = $size;
1248: $thumb_size = filesize($filename . '_thumb');
1249:
1250: // What about the extension?
==>1251: $thumb_ext = isset($context['valid_image_types'][$size[2]]) ? $context['valid_image_types'][$size[2]] : '';

The issue is that getimagesize can return boolean false on failure. When that happens, $size isn't an array and $size[2] will produce an error. The code needs to check the return value from getimagesize and do something appropriate if it's boolean false.

SpacePhoenix

The error suppression operator ( the @ ) has itself been suppressed so that all errors are displayed/logged

xdebug can be set so the the error suppression operator is ignored and the relevant error still gets displayed.

A question in StackOverflow covers it:

https://stackoverflow.com/questions/55928459/how-do-i-disable-error-suppression-operator-in-php

imo the error suppression operator should have been removed from PHP

Arantor

The @ has legitimate (if historical) use where you cannot possibly know in advance if an operation will fail until you try it - but failure guarantees an error message because proper error handling is still a mystery to PHP's devs in places.

For example, writing to a file, you literally cannot know before you try writing the file that you will *succeed* in writing the file. Because even if you do is_writable() beforehand, that doesn't verify that a) in the time between the is_writable() call and the write call that something hasn't changed the situation, especially on network shared drives, or even that b) there is sufficient space to write your file anyway.

Now, it's entirely possible to handle these failure states meaningfully after the fact but this is the sort of thing exceptions deal with, except that almost all of the file handling functions predate exception support, and changing them now breaks things because that's not how you do backward compatibility handling.

In this case though this isn't actually just a 2.1 bug, it's actually present in 2.0 as well if the topic display code has to regenerate the thumbnail but for whatever reason it's not actually able to (e.g. uploading a PSD)
Holder of controversial views, all of which my own.


live627

Quote from: SpacePhoenix on July 23, 2022, 09:56:43 AMThe error suppression operator ( the @ ) has itself been suppressed so that all errors are displayed/logged
where, on your server?

SpacePhoenix

Quote from: live627 on July 24, 2022, 01:25:31 AM
Quote from: SpacePhoenix on July 23, 2022, 09:56:43 AMThe error suppression operator ( the @ ) has itself been suppressed so that all errors are displayed/logged
where, on your server?

I use XDebug on my WAMP server and have got it configured so that it suppresses the error suppression operator

Kindred

So, how is your non-standard configuration a bug in the software?
Слaва
Украинi

Please do not PM, IM or Email me with support questions.  You will get better and faster responses in the support boards.  Thank you.

"Loki is not evil, although he is certainly not a force for good. Loki is... complicated."

Arantor

The bug is that the assumption is getimagesize() will have been fed an image it can work with and returned an array, but it won't always return an array. The @ is a distraction beause getimagesize() can also emit an error if given something that's not an image.

In this case the @ should even stay because you don't *know* that the thing is actually an image, but once the getimagesize call has been made, you can know to check it before the list() call.
Holder of controversial views, all of which my own.


SpacePhoenix

is_array() could be used on $size before the list() call with an appropriate error or exception generated if it's not an array

Arantor

But you know you still need to leave the @ there, right?
Holder of controversial views, all of which my own.


SpacePhoenix

Something has changed somewhere, as I downloaded and installed a clean copy, been messing around with it a very tiny amount.

Done a test attachment to a post which worked ok, and yet Subs-Attachments.php does not exist. Looks like ManageAttatchments.php has taken over its role

Arantor

That sounds like you're looking at 2.0 rather than 2.1.

The offending piece of code is in Display.php in 2.0 (since it's rebuild-thumbnail-in-thread), but in Subs-Attachments.php in 2.1 as per the OP.
Holder of controversial views, all of which my own.


SpacePhoenix

Still no error with 2.1. Both images that are loo large (both file size and dimensions) or invalid file types. Tried for both attachments and avatar upload. Also invalid file types.

Both PHP 7.3 and 7.4

Still with the suppression or the error suppression operator

Arantor

How are you testing, exactly? If you're doing this by way of uploading attachments, that's not enough.

Specifically the chunk of code is triggered by coming to the topic when an attachment that should have a thumbnail doesn't have a thumbnail - but the thing is also not able to be handled.

The sort of case I'm getting at is that you'd have to be at the topic itself, looking at an uploaded file (by way of the attach bbc), something that claimed to be an image - maybe even was an image originally - but for whatever reason fails in getimagesize. And that didn't have a thumbnail at the point of visiting the topic.

It's pretty specific to set up and test for.
Holder of controversial views, all of which my own.


SpacePhoenix

Tried uploading a file that's too big (image file). It uploads, no thumbnail. When clicked on gives File Not Found as a plain screen (no themes etc loaded). Still no PHP error.

Arantor

It's not the "too big" path that's the issue - it'll still have a valid size. You need to get into the workflow of "this is a file that to all intents and purposes should have a thumbnail" but when it actually comes time to make it, *it is not a valid image*. Too big doesn't make it not an image.
Holder of controversial views, all of which my own.


SpacePhoenix

Still not able to replicate it, any file that should generate a thumbnail still generates a thumbnail. Maybe the OP could upload a file that's known to causes the suspected issue

Arantor

We've actually spent more time debating if this is an issue than it would actually take to fix it.
Holder of controversial views, all of which my own.


sah62

Quote from: SpacePhoenix on July 25, 2022, 04:35:45 AMStill not able to replicate it, any file that should generate a thumbnail still generates a thumbnail. Maybe the OP could upload a file that's known to causes the suspected issue

I don't know what my forum user was doing to cause the issue. I just saw the error in my log. We know what the problem is, though: getimagesize can return boolean false, and when that happens "false" isn't an array. The easiest thing to do is to test the value of $size returned from getimagesize and do whatever is appropriate if it isn't an array. Perhaps something like this:

$size = @getimagesize($filename . '_thumb');
if (is_bool($size)) {
  // Do whatever is appropriate in the error situation.
}
else {
  list ($attachment['thumb_width'], $attachment['thumb_height']) = $size;
  $thumb_size = filesize($filename . '_thumb');

  // What about the extension?
  $thumb_ext = isset($context['valid_image_types'][$size[2]]) ? $context['valid_image_types'][$size[2]] : '';
  // ...and whatever is appropriate if there's no error.
}

sah62

Sorry, I went back to my error log and I noticed that the log includes the topic that my forum user was viewing. It contains three attachments. Unfortunately, when I viewed the topic all three attachments were properly displayed as thumbnails and no error was produced.

Steve

DO NOT pm me for support!

Arantor

No, it's a legitimate bug, albeit one that can't readily be reproduced.
Holder of controversial views, all of which my own.


sah62

I'm still seeing occasional instances of this error with SMF 2.1.4 and PHP 8.1.2. I decided to try a fix by changing this line of code in Subs-Attachments.php:

// What about the extension?
$thumb_ext = isset($context['valid_image_types'][$size[2]]) ? $context['valid_image_types'][$size[2]] : '';

to this:

// What about the extension?
$thumb_ext = is_array($size) && isset($context['valid_image_types'][$size[2]]) ? $context['valid_image_types'][$size[2]] : '';

That is, check to make sure that the value returned from getimagesize() and stored in $size is an array before attempting to access the value of $size[2]. If getimagesize() fails and returns boolean false, is_array($size) will evaluate to false and the value of $thumb_ext will be set to ''. That seems safe given that $thumb_ext will also be set to '' if isset($context['valid_image_types'][$size[2]]) evaluates to false.

Advertisement: