Saving an Image field with an image that has undeterminable dimensions throws PDOException

[jenlampton]

Some images do not return integer values for width and height values from GD. These images often render normally, and can even be resized by GD without an issue.

However, when the record for the image is being added to the respective database table, an exception is thrown because the width and height columns require integer values, and what is passed into the query is not valid.

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'field_[field name here]' at row 1...

From the Drupal issue:

All images [both those that work, and those that do not] were output by Adobe Photoshop and I've had no problems with them in other image manipulation environments.

Description of the bug

With a clean install, I can upload certain .jpg images and drupal will throw the following error:

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'field_image_width' at row 1: INSERT INTO {field_data_field_image} (entity_type, entity_id, revision_id, bundle, delta, language, field_image_fid, field_image_alt, field_image_title, field_image_width, field_image_height) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10); Array ( [:db_insert_placeholder_0] => node [:db_insert_placeholder_1] => 2 [:db_insert_placeholder_2] => 2 [:db_insert_placeholder_3] => article [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => und [:db_insert_placeholder_6] => 2 [:db_insert_placeholder_7] => [:db_insert_placeholder_8] => [:db_insert_placeholder_9] => [:db_insert_placeholder_10] => ) in field_sql_storage_field_storage_write() (line 448 of /my_directory/modules/field/modules/field_sql_storage/field_sql_storage.module)

This is one we inherited from Drupal.
See This Drupal 8 issue and This drupal 7 issue, that got marked as dupe.

Steps To Reproduce

To reproduce the behavior:

1. Install Backdrop
2. Upload the Problem_3.jpg file from #57
3. Try the Upload button, vs the node Submit button to see if there is a difference

Proposed resolution

This issue has two parts:

• a) getting a PDO exception on an integer field without an input value like width and height -- (can no longer be reproduced in Drupal 9)
• b) getting a PDO exception when uploading an invalid image, i.e. a text file renamed with an image extension or an image file of 0 bytes -- a duplicate of #2345695: Users are able to upload 0-byte images.

The resolution below attempts to fix only the first part -- saving valid values to the database.

Placeholder 6 is not an integer and is not NULL. There is some code in the function (lines 433-435 of field_sql_storage.module) that attempts to handle this:

  foreach ($field['columns'] as $column => $attributes) {
     $record[_field_sql_storage_columnname($field_name, $column)] = isset($item[$column]) ? $item[$column] : NULL;
  }

but for some reason this is not sufficient to convert these non-values to NULL before sending them to the database. I replaced this with the following:

  foreach ($field['columns'] as $column => $attributes) {
     //convert non-numeric values (such as empty strings) in integer fields to NULL
     if ($attributes['type'] == 'int' && !$attributes['not null'] && !is_numeric($item[$column])) {
       $item[$column] = NULL;
     }
     $record[_field_sql_storage_columnname($field_name, $column)] = isset($item[$column]) ? $item[$column] : NULL;
  }

and that has resolved the problem.

---

PHP version: is Version: 7.4.2
GD: bundled (2.1.0 compatible)

[jenlampton]

I've filed a pr at backdrop/backdrop#3222 that fixes this problem on my site.

[klonos]

I tried to reproduce this on a vanilla installation (our demo sandboxes), but I was able to see the problematic image in step 4 🤷 ...do we have another way to reproduce this?

[klonos]

OK, here's a trick to reproduce this:

1. create an empty (txt) file, but save it with a .jpg extension.
2. upload it to a new post node.
3. try to save the node -> error 👎

SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column field_image_width' at row 1`

4. create a txt file, add some dummy content in it (so that it's not 0 bytes), and also save it as .jpg

5. upload this new dummy file to a new post node.

6. try to save the node -> same error 👎

7. duplicate an existing, valid .jpg image, open it in a text editor, delete all its content, or replace it with some dummy content, then save it.

8. upload this new dummy file to a new post node.

9. try to save the node -> same error 👎

Try the above in the sandbox of @jenlampton's PR -> no errors 🙂

Now the question is: should we be allowing this, or should we be throwing errors/warnings after the "faulty" image has been uploaded to the field?

[indigoxela]

I could also reproduce it (with a text file with jpg extension). I can confirm that the PR fixes the Exception. (I've left comments at the PR).

the question is: should we be allowing this, or should we be throwing errors/warnings after the "faulty" image

I second that! If we allow uploading broken/faulty images, we only create follow-up problems.

[indigoxela]

Here's an alternate PR which strikes earlier.

I realized that core already has a handy verification function for images: file_validate_is_image
For whatever reason the image module uses that function only for verifying the default image, but not yet for the actual image fields. I guess, this has been an oversight, my PR adds this validator to the actual image field upload, too.

How to test:

Create a "broken" image file, for instance by changing the extension of a text file to jpg. (Linux users can do touch notanimage.jpg)

Then upload that "notanimage.jpg" into an image field. Instead of the Exception you get a form error message (which could get improved a bit): "Only JPEG, PNG and GIF images are allowed."

[indigoxela]

@klonos have you already been able to test my alternative PR? It prevents uploading of broken/faulty images (anything gd can't handle), just as you suggested.

[klonos]

@klonos have you already been able to test my alternative PR?

Just did, and I prefer it over @jenlampton's 👍

you get a form error message (which could get improved a bit)

Yup. Please see my PR against yours: indigoxela/backdrop#1

[indigoxela]

Please see my PR against yours: indigoxela/backdrop#1

Many thanks for that. Please see my question regarding your change in function file_validate_is_image().

[klonos]

Replied @indigoxela

[indigoxela]

@klonos discovered that image files with a wrong extension (e.g. bmp or webp with a jpg extension) are also broken after upload. These need a slightly different form error message. His code provides that. This Issue is ready for review.

[klonos]

@indigoxela yet another simple PR, to fix the failing test: indigoxela/backdrop#2

[jenlampton]

I can't tell from the recent comments if this also addresses the original problem: when images are uploaded that are perfectly fine (but GD still can't handle them).

If you read the original Drupal issues, you can see that the 'faulty' images there were generated in exactly the same way as the images that worked, but for some reason GD chokes on some of them and not on others. GD has a lot of known issues like this, so we should be careful that we don't end up using GD to determine whether an image is 'faulty' or not.

[klonos]

...images are uploaded that are perfectly fine (but GD still can't handle them)

We'd need example test images and/or GD versions that allow reproducing this. We cannot fix what cannot be reproduced 🤷 ...this PR does improve the handling and error messages thrown with known/reproducible problems.

[jenlampton]

this PR does improve the handling and error messages thrown with known/reproducible problems.'

But it also introduces a new problem: not being able to upload images that are perfectly fine. Since we already know that GD has this problem (even if we can't reproduce it here) it would be silly to introduce known bugs.

That said, I'll see if I can collect some of the logos off the site that is suffering from these issues.

We cannot fix what cannot be reproduced

But we cross-port the same fix Drupal includes. :)

[indigoxela]

it also introduces a new problem: not being able to upload images that are perfectly fine

@jenlampton are you referring to the "Problem.jpg" you provided?
That image is corrupted, but I still can upload it to an image field and gd (2.2.5) handles it fine, it gets displayed and derivatives get created without problem.

Here's a "second opinion" from Imagemagic (identify):

Problem.jpg JPEG 300x173 300x173+0+0 8-bit sRGB 60773B 0.000u 0:00.010
identify: Corrupt JPEG data: 4769 extraneous bytes before marker 0xee `Problem.jpg' @ warning/jpeg.c/JPEGWarningHandler/365.

I agree with @klonos - We cannot fix what cannot be reproduced. We do fix reproducible errors with our PR.

[jenlampton]

But we are introducing new ones too. Please read the related Drupal issues :)

…

On Mon, Aug 17, 2020, 11:34 PM indigoxela ***@***.***> wrote: it also introduces a new problem: not being able to upload images that are perfectly fine @jenlampton <https://github.com/jenlampton> are you referring to the "Problem.jpg" you provided? That image *is* corrupted, but I still can upload it to an image field and gd (2.2.5) handles it fine, it gets displayed and derivatives get created without problem. Here's a "second opinion" from Imagemagic (identify): Problem.jpg JPEG 300x173 300x173+0+0 8-bit sRGB 60773B 0.000u 0:00.010 identify: Corrupt JPEG data: 4769 extraneous bytes before marker 0xee `Problem.jpg' @ warning/jpeg.c/JPEGWarningHandler/365. I agree with @klonos <https://github.com/klonos> - We cannot fix what cannot be reproduced. We *do* fix reproducible errors with our PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4526 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBERYXP57UYXOVKFJZP23SBIOHLANCNFSM4PYEV62A> .

[indigoxela]

But we are introducing new ones too

@jenlampton sometimes I wish your comments provided more details. 😉

The situation:

Currently if gd can not extract essential information we get an Exception - a bad thing. Just catching that Exception does not really solve the problem. The image still won't display and no derivatives get created - gd can't handle the file.

But the Exception only appears with non-image files (like text) or severely broken images, not just "a little corrupt".
The first commit of our PR fixes that.

We also prevent images with wrong extensions to get uploaded even if gd could handle them technically, but they're not of type jpg/gif/png. For instance a file with jpg extension that's actually a webp or bmp can't get uploaded. Files that are actually png, gif or jpg, but with wrong extension still work - as they do currently. No change in that.

A webp or bmp file with jpg extension does currently not work as expected. We can upload stuff like that, no Exception on saving, but the image won't ever display.

Please also note that images that don't display also cause dblog messages: "Unable to generate the derived image located at public://styles/..." On every page call - I'm pretty sure we should prevent that - the base problem, not the message.

Do I get your concerns right that you suggest we should allow uploading of images that have wrong extensions?

Finally regarding the patch on drupal.org - it introduces a brand new function that does the same as file_validate_is_image(), which already exists in both, D7 and Backdrop. Using the already existing function makes more sense.

NOTE: I updated the comment to rectify the description of handling png/gif/jpeg with wrong extension - they still do work.

[indigoxela]

Here's a quick tip to create a severely broken "image": put the first ten bytes of a valid jpg to a new file.

On Linux head -c 10 real.jpg > firstjpgegbytes.jpg
This (sort of) simulates an interrupted upload or local disk corruption.

This file causes an Exception on current Backdrops but upload is refused with our PR:

"The specified file firstjpgegbytes.jpg could not be uploaded. The uploaded file is either corrupt or not an image."

[jenlampton]

@jenlampton sometimes I wish your comments provided more details.

Sorry, I thought I'd already stated everything as clearly as I could, and that maybe the Drupal issues would do a better job than I could, hence me deferring to them instead. I'll try to do better :)

Currently if gd can not extract essential information we get an Exception - a bad thing. Just catching that Exception does not really solve the problem. The image still won't display and no derivatives get created - gd can't handle the file.

that's not my use-case for a this ticket.

The use-case for this ticket is for images that are perfectly fine. They render well in a browser, and they get resized without an issue. In some cases, GD still chokes on getting a width and height for these images, and that can cause an exception.

From an end-user perspective, I don't care if GD can't read the width and height of those images. If they display fine, and they resize fine, I should be allowed to upload them to my site and/or save pages that already have them.

In my case, I have a site with 100s of logos (some are very old -- the site was upgraded from Drupal 6) and in most cases my client does not have a choice in the image they were given. They can't go back to their client and say "My CMS doesn't like this logo" -- their client will think they are nuts if the logo works for everyone else (or, worked on this site before this change).

Do I get your concerns right that you suggest we should allow uploading of images that have wrong extensions?

The Images with the wrong extensions bit was something that @klonos added in a later comment but has nothing to do with the problem I am facing.

[klonos]

The use-case for this ticket is for images that are perfectly fine. They render well in a browser, and they get resized without an issue. In some cases, GD still chokes on getting a width and height for these images, and that can cause an exception.

@jenlampton can you please provide a few of these images, so we can test?

[jenlampton]

I've updated the first post in this thread in an attempt to state the problem more clearly. I've included more of the text from the Drupal 7 issue (and less from the Drupal 8 issue) for a closer parallel to what I was facing.

I've also split the Proposed resolution section into two parts -- a) is the problem I was attempting to fix with the database write, and b) is the problem @klonos is attempting to fix for Images with the wrong extensions. That is also being addressed in #2345695: Users are able to upload 0-byte images.

(I'm currently working on new STR with more/better image examples.)

[indigoxela]

@jenlampton I'd suggest that I open a new Issue for the upload validation of image fields. And you move on with your own PR here. Is that OK for you? Mixing these two items here makes things complicated.

[indigoxela]

To further narrow down the problem with images that display but still cause an Exception it would be essential to also know the full php and gd version of your affected site(s) - admin/reports/status/php.

There must be a reason why your site chokes on it, but @klonos and me were not able to reproduce the error with "Problem_3.jpg".
Or if you can't reproduce it with Problem_3.jpg either but with other images, getting those images would be cool.

[klonos]

I'd suggest that I open a new Issue for the upload validation of image fields. And you move on with your own PR here. Is that OK for you? Mixing these two items here makes things complicated.

I'm supportive of this 👍

Or if you can't reproduce it with Problem_3.jpg either but with other images, getting those images would be cool.

Yup, @jenlampton can you please:

• a) confirm that Problem_3.jpg from the d.org issue is also problematic on those sites of yours that are throwing those PDOException errors?
• b) provide us with a few sample images that are causing your sites to choke? (along with php/gd versions etc)