Fix some division-by-zero problems in src\lib\openjp2\pi.c

[trylab]

The following issues were fixed in this commit.

#731
#732
#777
#778
#779
#780

[mayeut]

This would also fix some issues with images from #811

@detonin, Is the fix legal regarding jp2 standard or will that just postpone (not for those issues though) the moment when a crash will happen ? Are the values legal ? I f not, could they be checked rather than just continuing.

[trylab]

@mayeut The fix just solve the issues on the spot. I think there are two ways to solve them on the spot.

If division-by-zero problems are detected, just continue; or return OPJ_FALSE;. Since return OPJ_TRUE; only happens in a single if statement, I think continue is more preferable.

The root cause needs further investigate.

[mayeut]

All opj_pi_next_* shall probably be fixed (not all need a fix I think).
@detonin, I won't be able to investigate the root cause for this.
Two options:

• Merge this now & create an issue to identify the root cause.
• Wait for someone to identify the root cause & fix it

The second option is only to be considered if it can be done in the short term.

[malaterre]

I vote for option (2), it seems we are not doing sanity checks on inputs. See for example #731 (comment)

[trylab]

Hi, I uploaded a minimized poc file at #731 (comment)

[trylab]

After I did some investigation on this issue, I think it's hard to do sanity checks earlier.

The following j2k image was taken from the JPEG2000 Standard Document (Part 1) (ISO/IEC 15444-1). It's a legal j2k image.

FF 4F FF 51 00 29 00 00 00 00 00 01 00 00 00 09
00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 09
00 00 00 00 00 00 00 00 00 01 07 01 01 FF 5C 00
07 40 40 48 48 50 FF 52 00 0C 00 00 00 01 00 01
04 04 00 01 FF 90 00 0A 00 00 00 00 00 1E 00 01
FF 93 C7 D4 0C 01 8F 0D C8 75 5D C0 7C 21 80 0F
B1 76 FF D9

We can create a proof-of-concept file just by modifying three bytes.

FF 4F FF 51 00 29 00 00 00 00 00 01 00 00 00 09
00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 09
00 00 00 00 00 00 00 00 00 01 07 01 [02] FF 5C 00     // #1
07 40 40 48 48 50 FF 52 00 0C 00 [04] 00 01 00 [11]    // #2  #3
04 04 00 01 FF 90 00 0A 00 00 00 00 00 1E 00 01
FF 93 C7 D4 0C 01 8F 0D C8 75 5D C0 7C 21 80 0F
B1 76 FF D9

1. Change l_img_comp->dy to 0x02, this byte will be read in function opj_j2k_read_siz. The value will be propagated to comp->dy in function opj_pi_next_cprl. Values in interval [1,255] are all valid.
2. Change SGcod(A) to 0x04, the value is assigned to the Progression order field which can be used to control which opj_pi_next_* function will be called. The valid values can be 0,1,2,3,4. Here 4 is used to make sure that function opj_pi_next_cprl will be called.
3. Change first byte of SPcod to 0x11, the value is assigned to l_tccp->numresolutions in function opj_j2k_read_SPCod_SPCoc. Values in interval [0,32] are all valid. The value can be used to control levelno in function opj_pi_next_cprl.
4. Here the value of Scod is 0 which can be used to make sure that values of res->pdx and res->pdy in function opj_pi_next_cprl are all 0x0F. The 0x0F values are initialized in function opj_j2k_read_SPCod_SPCoc. Also, the value is legal according to the standard document.

Now we can figure out why the division-by-zero problem will be happened. So I think that all the values are legal and it's hard to do sanity checks earlier.

BTW, I'm not familiar with the whole standard. The above text is just my analysis. Please point out if something is wrong. Thanks.

[detonin]

@trylab thanks for the poc
@mayeut values are legal indeed but AFAIU, division by zero occurs because of an exceeding amount of left-shift operations ... So is it not OPJ_INT32 the problem here ?

[malaterre]

@detonin I do not believe so since kakadu is also failing later in the pipeline (as indicated here: #731 (comment))

[rouault]

I've fixed those issues in a somewhat equivalent way in d27ccf0 . Closing