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

Mon Feb 24 16:54:01 UTC 2025

On Fri, 21 Feb 2025 00:50:45 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"**_
>> 
>> Fix consists of:
>> * `private boolean isBuiltIn = false` in ICC_Profile which is set to true when BuiltIn profiles are created and false for non-builtIn profile.
>> * Converted BuiltInProfile from private interface to private static class (with all the profile instances as static final)
>> * `isBuiltIn` flag is set to true when BuiltInProfile are loaded.
>> * JavaDoc update for setData() (CSR required)
>> 
>> 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 represtation 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:
> 
>   javadoc update

Changes requested by aivanov (Reviewer).

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

> 1161:      * {@link ColorSpace#CS_PYCC}, {@link ColorSpace#CS_GRAY} or
> 1162:      * {@link ColorSpace#CS_CIEXYZ}.
> 1163:      * </p>

Suggestion:

     * <p>
     * Note: JDK built-in ICC Profiles cannot be updated using this method
     * as it will result in {@code IllegalArgumentException}. JDK built-in
     * profiles are those obtained by
     * {@code ICC_Profile.getInstance(int colorSpaceID)}
     * where {@code colorSpaceID} is one of the following:
     * {@link ColorSpace#CS_sRGB}, {@link ColorSpace#CS_LINEAR_RGB},
     * {@link ColorSpace#CS_PYCC}, {@link ColorSpace#CS_GRAY} or
     * {@link ColorSpace#CS_CIEXYZ}.
     * </p>

You should spell `IllegalArgumentException` fully, it explicitly lists the exception thrown. And `colorSpaceID` should also be marked up with `{@code}`.

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

> 1168:      * @throws IllegalArgumentException if {@code tagSignature} is not a
> 1169:      *         signature as defined in the ICC specification.
> 1170:      * @throws IllegalArgumentException if a content of the {@code tagData}

This is an existing issue, if it's an issue: should the text say *“**the** content”* instead of *“**a** content”*? @prrace

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

> 1173:      * @throws IllegalArgumentException if this is a profile for one of the
> 1174:      *         built-in pre-defined ColorSpaces, i.e. those which can be obtained
> 1175:      *         by calling {@code ICC_Profile.getInstance(int colorSpaceID)}

Suggestion:

     * @throws IllegalArgumentException if this is a built-in profile for one of the
     *         pre-defined color spaces, i.e. those which can be obtained
     *         by calling {@code ICC_Profile.getInstance(int colorSpaceID)}

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

> 40:         System.out.println("CASE 1: Testing BuiltIn Profile");
> 41:         updateProfile(true);
> 42:         System.out.println("Passed \n");

Suggestion:

        System.out.println("Passed\n");

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

> 49:     private static void updateProfile(boolean isBuiltIn) {
> 50:         ICC_Profile builtInProfile = ICC_Profile.getInstance(ColorSpace.CS_sRGB);
> 51:         //create a copy of the BuiltIn profile

Suggestion:

        // Create a copy of the built-in profile

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

> 56:         byte[] headerData = iccProfile.getData(HEADER_TAG);
> 57:         int index = PROFILE_CLASS_START_INDEX;
> 58:         //set profile class to valid icSigInputClass = 0x73636E72

Suggestion:

        // Set profile class to valid icSigInputClass = 0x73636E72

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

> 64:         if (isBuiltIn) {
> 65:             try {
> 66:                 //try updating a BuiltIn Profile, the following stmt should throw IAE

Suggestion:

                // Try updating a built-in profile, IAE is expected

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

> 66:                 //try updating a BuiltIn Profile, the following stmt should throw IAE
> 67:                 iccProfile.setData(HEADER_TAG, headerData);
> 68:                 throw new RuntimeException("Test Failed ! IAE NOT thrown.");

Suggestion:

                throw new RuntimeException("Test Failed! IAE NOT thrown.");

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

> 74:             }
> 75:         } else {
> 76:             //modifying custom profile should NOT throw IAE

Suggestion:

            // Modifying custom profile should NOT throw IAE

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

> 31:  */
> 32: public final class ICC_ProfileSetNullDataTest {
> 33:     private static final int[] colorSpace = new int [] {

Suggestion:

    private static final int[] colorSpace = new int[] {

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

> 46:             } catch (IllegalArgumentException e) {
> 47:                 return;
> 48:             }

The test won't run for **all** the cases if you after the first successful case — use `continue`.

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

PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2637741457
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968002547
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968004976
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968009066
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968033857
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968022912
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968028128
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968027136
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968027393
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968028624
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968029468
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1968032947