vips thumbnail always fails on warnings [8.13.x]

[kstanikviacbs]

Based on my findings it seems like calling vips thumbnail with 8.13.x with images causing vips warnings leads to vips failing and stopping. This was not the case before (8.12.x). Also - passing fail-on parameter to overwrite the default one: (none) has no effect.

Could it be considered a bug introduced somehow by that change: f06c9f3 or control over failing logic has changed in another way and I've missed that?

[jcupitt]

Hi @kstanikviacbs,

Ooof that's annoying, I thought we'd fixed this. I agree, it's not working properly, I see:

$ vips thumbnail broken.jpg[fail-on=warning] x.jpg 200 
(vips:1321105): VIPS-WARNING **: 09:57:54.589: read gave 2 warnings
(vips:1321105): VIPS-WARNING **: 09:57:54.589: VipsJpeg: Premature end of JPEG file
$ echo $?
0

I'll have a look.

[kstanikviacbs]

Thanks, I really appreciate the quick reaction.

[jcupitt]

Ah it's the thumbnail fail-on, as you guessed. You now need to set fail_on at that level. Try:

$ vips thumbnail broken.jpg x.jpg 200 --fail-on warning

(vips:1325101): VIPS-WARNING **: 10:27:50.401: error in tile 0 x 96

(vips:1325101): VIPS-WARNING **: 10:27:50.401: error in tile 0 x 104

(vips:1325101): VIPS-WARNING **: 10:27:50.401: error in tile 0 x 30
VipsJpeg: Premature end of input file
error buffer: VipsJpeg: Premature end of input file
$ echo $?
1

[kstanikviacbs]

The thing is that no matter which fail-on I set (even: vips thumbnail corrupted.jpg test.jpg 200 --fail-on none), result is 1 and I no longer get an image as an output while processing broken image, it's basically just empty file. Calling vips thumbnail corrupted.jpg test.jpg 200 with 8.12.x generates renderable JPG output.

[jcupitt]

It's working for me. With this test JPG:

I see:

$ head -c 200000 nina.jpg > broken.jpg
$ rm x.jpg
$ vips --version
vips-8.13.1
$ vips thumbnail broken.jpg x.jpg 200 

(vips:1335612): VIPS-WARNING **: 12:44:14.553: read gave 2 warnings

(vips:1335612): VIPS-WARNING **: 12:44:14.553: VipsJpeg: Premature end of JPEG file

error buffer: VipsJpeg: Premature end of JPEG file
$ echo $?
0

And it writes:

Could you share your test image?

[kstanikviacbs]

Sure thing, please give that one a try:

[kstanikviacbs]

I started wondering if this has something to do with some changes introduced in handling Huffman code errors which is the case for this one from above. Perhaps it might be related to libjpeg instead, right?

[jcupitt]

With your file I see:

$ vips copy broken.jpg x.jpg
VipsJpeg: Corrupt JPEG data: bad Huffman code
VipsJpeg: Corrupt JPEG data: bad Huffman code

So there's an error in the header, not in the pixel data. Without a header, there's not much libvips can do.

I wonder how firefox is displaying it? I suppose they have some extra code to fix up corrupt huffman codes.

[kstanikviacbs]

Interesting fact is that exactly the same image works with 8.12.x.

[kstanikviacbs]

Ok, there's one thing that's different, 8.12.2 treats it like a warning:

$ vips -v
vips-8.12.2-Thu Feb 10 17:18:53 UTC 2022
 $ vips thumbnail corrupted.jpg test.jpg 200

(vips:15): VIPS-WARNING **: 11:53:19.198: read gave 39632 warnings

(vips:15): VIPS-WARNING **: 11:53:19.198: VipsJpeg: Corrupt JPEG data: bad Huffman code

[jcupitt]

Ah, it's this change:

https://github.com/libvips/libvips/blob/master/libvips/foreign/jpeg2vips.c#L332-L333

ie. more than 100 warnings counts as an error. This change was needed to prevent a class of denial-of-service attack: you can make jpg decodes extremely slow (as in many, many minutes) by deliberately making a file which will trigger 10000s of warnings.

[jcupitt]

Here's the issue: #2749

[kstanikviacbs]

Ach, gotcha, thanks for clarification. Well, I'm afraid I need to adjust that parameter for my use case, it's not exposed to API in any way right? The only way to go for me would be to change it on my side and build from sources, correct?

[jcupitt]

Other loaders have an unlimited option to disable DoS limits, perhaps the JPEG loader should as well.

But yes, until we add it, you'll have to patch the sources yourself.

[kstanikviacbs]

Understood, thanks for all your support, this is really helpful.

Would be great to have such option there as well at some point in the future. For the time being - I would discuss options with the team and potentially patch the sources by myself.

[jcupitt]

I had a few minutes, so I added unlimited to 8.13, this change will be in 8.13.1. We shouldn't add API in a stable branch, but I think this is necessary to fix a regression.

[jcupitt]

(and thanks for reporting this issue!)