Skip to content

openjp2/j2k: Report error if all wanted components are not decoded.#1164

Merged
rouault merged 1 commit intouclouvain:masterfrom
sebras:master
Sep 3, 2019
Merged

openjp2/j2k: Report error if all wanted components are not decoded.#1164
rouault merged 1 commit intouclouvain:masterfrom
sebras:master

Conversation

@sebras
Copy link
Copy Markdown
Contributor

@sebras sebras commented Oct 31, 2018

Previously the caller had to check whether each component data had
been decoded. This means duplicating the checking in every user of
openjpeg which is unnecessary. If the caller wantes to decode all
or a set of, or a specific component then openjpeg ought to error
out if it was unable to do so.

Fixes #1158.

@sebras sebras force-pushed the master branch 5 times, most recently from 49cb46c to 2e01c17 Compare November 2, 2018 13:35
@sebras sebras changed the title openjp2/jp2: Validate all SGcod and SPcod parameters. openjp2/j2k: Report error if all wanted components are not decoded. Nov 2, 2018
@sebras sebras force-pushed the master branch 4 times, most recently from fb1807e to 2e1f95d Compare November 2, 2018 14:36
@sebras
Copy link
Copy Markdown
Contributor Author

sebras commented Nov 2, 2018

After several iterations I figured out that the latter commit was the only one needed to resolve this issue, and that the prior commit actually requires more input. I'll submit that separately later

@rouault
Copy link
Copy Markdown
Collaborator

rouault commented Nov 16, 2018

This is reasonable but a change in behaviour. Perhaps there are people who deal with corrupt images, and are used to try to recover the non-corrupted components. Although they will have the workaround of decoding each component individually. @detonin thoughts about this ?

@sebras
Copy link
Copy Markdown
Contributor Author

sebras commented Dec 7, 2018

@rouault: The current check in opj_decompress only checks the first component to determine if all components have been sucessfully decoded. That doesn't look right, and the comment above that check indicated that this ought to be done inside the library for the benefit of all users.

Also note that while this is a behavioural change it only affects clients decoding broken images where the decoding of one or more components fails and the client wants to keep the other components. Is this a common situation?

@rouault
Copy link
Copy Markdown
Collaborator

rouault commented Dec 7, 2018

We can go with your approach and see if people complain. However, I think a similar check should be added to opj_j2k_decode_one_tile(), no ? (in which case, please create a common function called by the 2 call sites)

@sebras
Copy link
Copy Markdown
Contributor Author

sebras commented Dec 7, 2018

@rouault Thanks for the reply. Yes, that does seem like an omission. I'll update the commit in a bit.

@sebras sebras force-pushed the master branch 3 times, most recently from 4950c38 to e692210 Compare December 8, 2018 16:07
Previously the caller had to check whether each component data had
been decoded. This means duplicating the checking in every user of
openjpeg which is unnecessary. If the caller wantes to decode all
or a set of, or a specific component then openjpeg ought to error
out if it was unable to do so.

Fixes uclouvain#1158.
@sebras
Copy link
Copy Markdown
Contributor Author

sebras commented Sep 3, 2019

I did address the review comments long while back. What needs to be done for this to be merged?

The error from continuous-integration/appveyor/pr failed, but it seems to have failed due to a network outage:

Build started
git clone -q --depth=50 https://github.com/uclouvain/openjpeg.git C:\projects\openjpeg
fatal: unable to access 'https://github.com/uclouvain/openjpeg.git/': Could not resolve host: github.com
Command exited with code 128

@rouault rouault merged commit e66125f into uclouvain:master Sep 3, 2019
@rouault
Copy link
Copy Markdown
Collaborator

rouault commented Sep 3, 2019

Thanks. Merged

@sebras
Copy link
Copy Markdown
Contributor Author

sebras commented Sep 3, 2019

Excellent, thank you! :)

chris-liddell pushed a commit to ArtifexSoftware/thirdparty-openjpeg that referenced this pull request Sep 5, 2019
Ghostscript used to attempt to use even the undecoded components.
The source code for upstream's opj_decompress tool avoided this by
a workaround along with a comment indicating that this ought to be
done in the library (so all clients, e.g. Ghostscript will benefit
from it). With this commit the library will error out if not all
requested components are successfully decoded. Thus Ghostscript
will no longer crash.

Reported in uclouvain/openjpeg#1158
sent upstream in uclouvain/openjpeg#1164
and finally committed in e66125f
chris-liddell pushed a commit to ArtifexSoftware/thirdparty-openjpeg that referenced this pull request Feb 6, 2020
The original commit message read:

Bug 700088: Report error if all wanted J2K components are not decoded.

Ghostscript used to attempt to use even the undecoded components.
The source code for upstream's opj_decompress tool avoided this by
a workaround along with a comment indicating that this ought to be
done in the library (so all clients, e.g. Ghostscript will benefit
from it). With this commit the library will error out if not all
requested components are successfully decoded. Thus Ghostscript
will no longer crash.

Reported in uclouvain/openjpeg#1158
sent upstream in uclouvain/openjpeg#1164
and finally committed in e66125f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heap buffer overflow in opj_t1_clbl_decode_processor() triggered with Ghostscript

2 participants