On Thu, 15 Oct 2026 02:45:54 GMT, Sergey Bylokhov <[email protected]> wrote:
>>> > Yes it does. I referenced the exact place in the JPEG spec that requires
>>> > it.
>>> > any use of 16 bit quantization with 8 bit samples is a bug in what ever
>>> > provides it or whatever allows it.
>>>
>>> That is correct. But neither "8 bit samples" nor this clamp makes the
>>> plugin baseline-only, before or after the patch. It is not, since
>>> progressive is supported. So the reason of the fix should be updated: not
>>> "ImageIO's native JPEG library is compiled to support baseline JPEG only",
>>> but "drop the libjpeg extension (16 bit quantization tables for 8 bit
>>> samples) which the plugin supported since 1.4". Baseline has nothing to do
>>> with it, and that is a behavior change.
>>
>> 8 bit samples is what is important. Progressive 8 bit does not change
>> anything.
>> The plugin has never *supported* 16 bit quantization. It not "preventing" it
>> is not the same thing as supported.
>>
>>> If the clamp on the read path is a guard against values outside 1..255, it
>>> does not work: the table stored in the image is read by libjpeg itself and
>>> reaches the decoder as is, the code change in this PR never sees it. To
>>> guard that the library must be changed, which is impossible with the system
>>> libjpeg. And the decoder does not need it, it only multiplies by the table,
>>> the crash is in the encoder. So the 255-clamp might be applied on write
>>> only.
>>
>> What crash ? I'm not seeing any crash, nor fixing a crash. I'm just fixing
>> the out of range values.
>> The null de-ref crash was the Huffman tables problem, not related to this.
>> The alternative would be to reject such an image which doesn't seem
>> necessary.
>
>>8 bit samples is what is important. Progressive 8 bit does not change
>>anything.
>
> I think we talk about different things. My initial point was only this:
>>So I do not think this is accurate:
>>ImageIO's native JPEG library is compiled to support baseline JPEG only. This
>>means 8 bit quantization values in the range 1 -> 255.
>
> "baseline JPEG only" in that text is not correct. The library is not compiled
> for baseline only, otherwise progressive would have to be dropped as well. It
> is compiled for 8 bit samples, and that is what the PR description should
> say, and that is what I pointed out, so that argument is not used as a reason
> later.
>
> It would be good to document this in JBS and possibly by the release-note,
> since this is a behavior change (dropping an extended behavior of libjpeg),
> not only a bug fix.
>
>>What crash ? I'm not seeing any crash, nor fixing a crash.
>
> ... as usual bugs just stick to me....
> I thought this clamp was a guard, and checking that code path I found that
> with some specific values it causes a crash. Your "< 1" clamp fixes it.
I changed the description to say "8 bit samples"
Do you have a test that shows this crash ?
I experimented just now with only clamping the < 1 .. but of course my new test
fails with that because the *existing* behaviour of the plugin is to throw an
Exception when reading a JPEG with 16-bit quantization data.
And by not clamping on write, that's what is there to be read.
I could make it do the < 1 clamp on read AND write, but only do the 255 clamp
on write.
- if (q > 255) { // ImageIO supports baseline only
+ if ((q > 255) && !cinfo->is_decompressor) { // ImageIO supports 8
bit samples only
q = 255;
}
in which case the test would pass.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33217#discussion_r4298594920
