RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v24]

[Alexey Ivanov]

On Thu, 10 Apr 2025 04:15:31 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>> Built-in Profiles are singleton objects and if the user happens to modify this shared profile object via setData() then the modified version of the profile is returned each time the same built-in profile is requested via getInstance().
>> 
>> It is good to protect Built-in profiles from such direct modification by adding BuiltIn profile check in `setData()` such that **only copies** of Built-In profiles are allowed to be updated.
>> 
>> With the proposed fix, if Built-In profile is updated using `.setData()` it throws _**IAE - "BuiltIn profile cannot be modified"**_
>> 
>> There are no restrictions on creating copies of BuiltIn profile and then modifying it, but what is being restricted with this fix is - the direct modification of the shared BuiltIn profile instance.
>> 
>> Applications which need a modified version of the ICC Profile should instead do the following:
>> 
>> 
>> byte[] profileData = ICC_Profile.getData() // get the byte array representation of BuiltIn- profile
>> ICCProfile newProfile = ICC_Profile.getInstance(profileData) // create a new profile
>> newProfile.setData() // to modify and customize the profile
>> 
>> 
>> Following existing tests are modified to update a copy of Built-In profile.
>> 
>> - java/awt/color/ICC_Profile/SetHeaderInfo.java
>> - java/awt/color/ICC_ProfileSetNullDataTest.java
>> - sun/java2d/cmm/ProfileOp/SetDataTest.java
>
> Harshitha Onkar has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 26 additional commits since the last revision:
> 
>  - Merge branch 'master' into BuiltInCheck
>  - added javadoc to testprofile
>  - single case type
>  - added test cases to read .icc, updated test code
>  - serialization test case changes
>  - Merge branch 'master' into BuiltInCheck
>  - updated test to check for all builtIn profiles, serial-deserialization
>  - redudant stmt removed
>  - modifier order changed, added comment to BuiltInProfile
>  - review changes
>  - ... and 16 more: https://git.openjdk.org/jdk/compare/55b4ca24...b55bebf2

Looks good to me, yet there are a few small typos, fixes…

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 113:

> 111:     /**
> 112:      * Set to {@code true} for {@code BuiltInProfile}, {@code false} otherwise.
> 113:      * This check is used in {@link #setData(int, byte[])} to prevent modifying

Suggestion:

     * This flag is used in {@link #setData(int, byte[])} to prevent modifying

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 126:

> 124:         /*
> 125:          * ProfileDeferralInfo is used for only built-in profile creation and
> 126:          * all built-in profiles should be constructed using it.

Suggestion:

         * ProfileDeferralInfo is used for built-in profile creation only, and
         * all built-in profiles should be constructed using it.

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 783:

> 781:      * <p>
> 782:      * Note: {@code ProfileDeferralInfo} is used for only built-in profile
> 783:      * creation and all built-in profiles should be constructed using it.

Suggestion:

     * Note: {@code ProfileDeferralInfo} is used for built-in profile
     * creation only, and all built-in profiles should be constructed using it.

test/jdk/java/awt/color/ICC_Profile/BuiltInProfileCheck/BuiltInProfileCheck.java line 154:

> 152:                 // Try updating a built-in profile, IAE is expected
> 153:                 testProfile.setData(HEADER_TAG, headerData);
> 154:                 throw new RuntimeException("Test Failed! IAE NOT thrown for profile"

Suggestion:

                throw new RuntimeException("Test Failed! IAE NOT thrown for profile "

test/jdk/java/awt/color/ICC_ProfileSetNullDataTest.java line 48:

> 46:             try {
> 47:                 profile.setData(ICC_Profile.icSigCmykData, null);
> 48:                 throw new RuntimeException("IAE expected, but not thrown for"

Suggestion:

                throw new RuntimeException("IAE expected, but not thrown for "

-------------

Marked as reviewed by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2757457605
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r2037793357
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r2037796650
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r2037797607
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r2037823276
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r2037822231