Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JDK-8347377 : Add validation checks for ICC_Profile header fields #23044
base: master
Are you sure you want to change the base?
JDK-8347377 : Add validation checks for ICC_Profile header fields #23044
Changes from 7 commits
f17cdd6
3ef30ad
550506d
d3a4b8b
ffacf6f
8243687
04880f6
a5201b5
b31900b
78500c2
5aa82b2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't run into ArrayIndexOutOfBoundsException here since the incoming data array size is already being verified in ProfileDataVerifier.verify(data).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can any of these fields have custom values (not covered by java constants inside iccCStoJCS) that can still be used for color transformation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using custom values of color space to create a new color transform (For instance: cmsSigMCH1Data: 0x4D434831 which is specified in LCMS API but not in ICC Spec) results in ProfileDataException as below. (this is expected since cmsSigMCH1Data constant is NOT present in JDK).
To summarize:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how it is handled by the lcms library? don't we need to ignore unknown intents(and other parameters) and lets lcms decide what to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrserb
Non-header data are updated using cooked approach (and validated by LCMS) whereas header data are updated using raw LCMS APIs hence require additional validation before setData() is called (On native side it is handled here: setTagDataNative() in LCMS.c).
Without the fix, if invalid rendering intent, PCS, ColorSpace or Device class is updated using setData() it does not throw IAE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, LCMS ignores invalid header data and updates the profile, this can cause exceptions later on for instance when the modified profile is used to create BufferedImage which can be prevented by adding checks in
setData()
and restricting updates to only allowed values as specified in ICC Spec Doc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ICC spec. defines only these 4 intents, so I don't see a problem here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then probably we can use approach similar to 8282577: f66070b and try to rely on lcms for validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW
Why this usecase should not be covered by the java? As of the current version of patch it will not be possible to load the profile and then set the intent, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path that you mentioned in LCMSTransformfor RenderingIntent involves creating a new color space transform using 2 or more profiles and does not involve creating or updating a profile.
Moreover when a random value is added for rendering intent in LCMSTransform, LCMS throws CMMException. If LCMS validates the Rendering Intent while creating a new color space transform then wouldn't it be better to validate it while creating/updating a ICC_Profile?
And as for the different values specified in ICC_Spec vs LCMS API doc, @prrace confirmed that jdk is required to follow ICC_Spec. In other words the color mgnt engine can have different implementations (LCMS or KCMS) but we need to follow the ICC Specification.
Discussed with @prrace, in this case it was decided that contacting upstream is not necessary since we are choosing to do validations in any case (whether or not LCMS validates).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrserb
I see your point but color management engines may have different implementations (LCMS or KCMS) and ICC Specification is a standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure the lcms/kcms follow the standard, because of the next statement in the "ICC.1-2022-05.pdf"
Since you already mentioned "non-ICC intent", please specify the part of the specification in ICC.1-2022-05 where their use is prohibited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrserb
The ICC Spec doesn't explicitly say that certain values are prohibited. Although it does not list Non-ICC Intent values under Rendering Intent (pg#23, Table 23) either.
What do you suggest is the best solution here to address the difference in ICC Spec Doc vs LCMS API doc?
@prrace Can you please suggest how to address Sergey's concern, since the last time we discussed we agreed to follow ICC Spec Doc.