Avoid warning from imagecreatefrompng when loading png with obscure iCCP profiles

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Could you please provide steps/configuration taken to recreate this.
This will need a test case showing the issue.

Thanks

I was able to recreate this by uploading a .webp image into an image/media reference field, and then rendering it using an image style.

I was able to extract the broken color profile from the image and apply it to another one and trigger that error in tests. The broken color profile called sRGB IEC61966-2.1. I'm attaching it as well, so you can test it by yourself on any image (by injecting it using CLI tools or image editors like GIMP, Krita, etc).

I'm also switch versions to 10.1.x, because patch from #8 won’t apply on 10.0.x branch. And I focus initially on future branch, if it's needed it can be backported later on.

Updated issue summary with steps to reproduce this problem.

Added cspell ignore for iCCP

Sorry for noise, patch in #20 contains .icc profile by accident. This one is clean patches.

Gosh 🤯

Anyway, since I need to create patches again, I generated a test image using convert instead of GIMP. It is smaller, because there is no GIMP metadata in it.

🤞

The last submitted patch, 22: drupal-3261924-22-test-only.patch, failed testing. View results →

Personally, I would not create tests for suppression operators. Such tests are too tricky.
Also, I think it needs a comment explaining what kind of errors could happen without the suppression.

Added comment for suppression call.

Disabled tests, because the only difference from #22 is comment.

Re-rolled patch #3 to make it work with 9.5.5

#26 works! no more warning on d 9.5.5/php 8.1.1
thank you!

The issue title is used as the commit message and it should state what this issue resolves instead of being a long error message.

Do we really want to suppress all warnings?

Backporting #8 to D7

Better title.

Do we really want to suppress all warnings?

I had the same question, but I'm not sure what options we're really given. I suspect this is probably OK though because real problems will throw and exception which gets caught and logged. At least in the 10.x versions of this patch.

That said, the block we're adjusting is generalized for all image formats but the comment clearly only addresses the impact on PNGs. Very bikesheddy but maybe we should probably adjust the documentation to match the scope of the code not the scope of this issue. Something like this?

      // Suppress warnings from image creation because there is a possibility
      // that source images contain data that would trigger non-fatal warnings
      // we can't handle. For example, a PNG saved with 'sRGB IEC61966-2.1'
      // color profile has 'iCCP' chunk data which is not recognized on most
      // systems and will cause libpng to trigger warnings but otherwise do not
      // affect image processing.

Looking at #25 and should the test go into ToolkitGdTest?

Also reading the last comment 31 think expanding to match makes sense. Least I can't think of a reason not to.

Rerolled the last patch against 11.x.

Rerolled the last patch against 10.1.2

I confirm the patch in 34 🐛 Avoid warning from imagecreatefrompng when loading png with obscure iCCP profiles Needs work works for Drupal 10.

Based on the comment from #35, I'm changing the status to RTBC.

Based on the rest of the comments and the lack of tests don't think this is actually ready. There aren't even tests on the last couple patches.

re: #32 what the heck is that test. Its not in the system module, its in the wrong location based on the namespace of the class, the file name doesn't align with the tested class? Something weird going on there but it does look like that's where existing tests are.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Taran2L → changed the visibility of the branch 3261924-avoid-warning-from to hidden.

Taran2L → changed the visibility of the branch png-11.x to hidden.

1 small feedback item.

Hiding patches for clarity.

@smustgrave I disagree, the dictionary already has all the wierd words ...

Which is actively trying to be cleaned up

Question: is it possible to resolve threads ?

There was 1 error:
1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testIncorrectIccpSrgbProfile with data set "PNG with iCCP profile" ('core/tests/fixtures/files/ima...le.png')
PHPUnit\Framework\Exception: PHP Warning:  imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 271
Warning: imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 271
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/TextUI/Command.php:97
ERRORS!
Tests: 97, Assertions: 1197, Errors: 1.

Verified test coverage using test-only feature.

All feedback has been addressed.

@Taran2L as far as threads, as the opener of the thread I use to be able to close myself but that no longer seems to be the case. Think only the person who opened the MR can resolve threads now.

I'm triaging RTBC issues → . I read the IS, the comments and the MR. Thanks for the issue summary. As someone who doesn't work with images it was very clear and easy to understand the problem. I didn't find any unanswered questions.

I read the MR (not a code review) and there is minor work to do on the comments.

Oh sorry, @neclimdul, thank you for answering my question in #31.

Rerolled patch from #34 for Drupal 10.2

Taran2L → changed the visibility of the branch 10.2.x to hidden.

Feedback has been addressed

Also, attaching a static patch from the latest changes in the MR (for composer patching purposes)

MR appears to have failures.

Feedback appears to have been addressed

Attaching static version of MR for composer patch.

The test doesn't work for me locally. With or without the change to GDToolkit I get:

Testing Drupal\KernelTests\Core\Image\ToolkitGdTest
..E                                                                 3 / 3 (100%)

Time: 00:03.934, Memory: 4.00 MB

There was 1 error:

1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testIncorrectIccpSrgbProfile with data set "PNG with iCCP profile" ('core/tests/fixtures/files/ima...le.png')
PHPUnit\Framework\Exception: libpng warning: iCCP: known incorrect sRGB profile

@longwave this is very weird - it does not work on my local as well ... but it seems it does work on CI, weird

I think the value of the test is that error is being suppressed.

Hello again. I see some folks not very happy with silencing all the errors from that call. I understand that as well, it can be helpful sometimes. Without it, I won't be found this issue as well. Maybe we wrap this call $image = @$function($this->getSource()); by a condition?

We have already configuration `system.logging` with `error_level` setting. Maybe we will do it like:

if ($error_level === 'verbose') {
  $image = $function($this->getSource());
}
else {
  $image = @$function($this->getSource());
}

This allows a developer to find out that there is a problem, but it won't be logged and showed on production instances. It might be looking ugly, but it solves most of the concerns here. Currently, as I remember correctly (I've already fixed that images), this will be logged on production on every over page with a broken image when it's processed for the first time (if no style yet created). In some cases, it can create a lot of useless noise in the logging system.

It IS possible to handle warnings, and therefore forego suppressing them all, but it would require setting up an error handler to do it. That said, if we are worried about particular image profiles, is there a way to whitelist some and check to see if the image is what we want? Is that too process intensive?

Trying to throw some fresh ideas at this that I didn't see in the comments. I do like Niklan's thoughts though.

I think the concerns might be a bit overstated. Actual problems will trigger real exceptions that get logged. This just avoids warnings that shouldn't be triggered. If I remember correctly they're warnings triggered by the underlying libraries so they don't actually follow PHP's documentation for the method.

Resolved MR conflict and added patches for D10 and D11.

Just curious about Patch 65. What's all the garbage text at the end of the file?

That's a new file... png based on the diff.

diff --git a/core/tests/fixtures/files/image-test-iccp-profile.png b/core/tests/fixtures/files/image-test-iccp-profile.png
new file mode 100644
index 0000000000000000000000000000000000000000..29cb8945f8e4c1ae40de24bf888715e4d870a164
--- /dev/null
+++ b/core/tests/fixtures/files/image-test-iccp-profile.png
@@ -0,0 +1,24 @@
+‰PNG

Trying to bring all changed into new branch as pull from 11.x returns too many merge conflict. 🤷‍♀️
Also, changing branch to 11.0.x

VladimirAus → changed the visibility of the branch 3261924-avoid-warning-from-d11.0 to hidden.