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

[Alexey Ivanov]

On Mon, 10 Mar 2025 23:22:36 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 incrementally with one additional commit since the last revision:
> 
>   builtIn converted to transient, tests updated

Changes requested by aivanov (Reviewer).

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

> 1172:         if (builtIn) {
> 1173:             throw new IllegalArgumentException("Built-in profile"
> 1174:                                                + " cannot be modified");

Is it acceptable to overshoot 80-column limit for 6 characters without wrapping the line?

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

> 67:                 throw new RuntimeException("Test Failed! IAE NOT thrown.");
> 68:             } catch (IllegalArgumentException iae) {
> 69:                 System.out.println("IAE expected: " + iae.getMessage());

I'm for keeping the previous version which verified the exception message — otherwise, how can we distinguish `IllegalArgumentException` for illegal arguments?

If we used another exception type, there wouldn't be the need to check on the message.

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

> 39:             ColorSpace.CS_CIEXYZ, "CS_CIEXYZ",
> 40:             ColorSpace.CS_LINEAR_RGB, "CS_LINEAR_RGB"
> 41:     ));

Suggestion:

    private static final Map<Integer, String> colorSpace = Map.of(
            ColorSpace.CS_sRGB, "CS_sRGB",
            ColorSpace.CS_PYCC, "CS_PYCC",
            ColorSpace.CS_GRAY, "CS_GRAY",
            ColorSpace.CS_CIEXYZ, "CS_CIEXYZ",
            ColorSpace.CS_LINEAR_RGB, "CS_LINEAR_RGB"
    );


Printing the `int` value of a color space could be enough, although a readable name is better.

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

> 47:             ICC_Profile profile = ICC_Profile.getInstance(builtInProfile.getData());
> 48:             try {
> 49:                 profile.setData(ICC_Profile.icSigCmykData, tagData);

Suggestion:

                profile.setData(ICC_Profile.icSigCmykData, null);

Should this be simplified?

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

PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2674550615
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1989502914
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1989287031
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1989489808
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1989491425