search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

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

Open jenlampton opened 4 years ago

jenlampton commented 4 years ago

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:

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 commented 4 years ago

I've filed a pr at https://github.com/backdrop/backdrop/pull/3222 that fixes this problem on my site.

klonos commented 4 years ago

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 commented 4 years ago

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`

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

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

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

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

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

  6. 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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: https://github.com/indigoxela/backdrop/pull/1

indigoxela commented 4 years ago

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 commented 4 years ago

Replied @indigoxela

indigoxela commented 4 years ago

@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 commented 4 years ago

@indigoxela yet another simple PR, to fix the failing test: https://github.com/indigoxela/backdrop/pull/2

jenlampton commented 4 years ago

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 commented 4 years ago

...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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

On Mon, Aug 17, 2020, 11:34 PM indigoxela notifications@github.com 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 https://github.com/backdrop/backdrop-issues/issues/4526#issuecomment-675284125, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBERYXP57UYXOVKFJZP23SBIOHLANCNFSM4PYEV62A .

indigoxela commented 4 years ago

But we are introducing new ones too

@jenlampton sometimes I wish your comments provided more details. :wink:

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.

broken-image

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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:

indigoxela commented 2 months ago

Too much has changed in core's image handling, so I closed my outdated PR - there hasn't been any feedback for years, anyway.

klonos commented 2 months ago

Too much has changed in core's image handling ...

Indeed 👍🏼

...in the meantime, things have also progressed in the Drupal issues referenced in the issue summary:

So here's a PR that takes all that into account: https://github.com/backdrop/backdrop/pull/4807

The PR does the following:

klonos commented 2 months ago

@jenlampton do you still experience the problem you mentioned where valid images are reported as broken/corrupt or not properly handled by GD? If so, then can you provide a couple of these files as samples and let us know versions of PHP and GD on these servers?