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

[Alexey Ivanov]

On Wed, 19 Feb 2025 22:54:15 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 change

Changes requested by aivanov (Reviewer).

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

> 114:      * BuiltInProfile.
> 115:      */
> 116:     private boolean isBuiltIn = false;

Can't this field be `final` and be set in constructor of the class?

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

> 1152:      * This method is useful for advanced applications which need to access
> 1153:      * profile data directly. Only non-built-in, application provided profiles
> 1154:      * should be updated using this method.

What you mean is that only non-built-in profiles *can* be updated. The fix makes it impossible to update built-in profiles.

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

> 1162:      *         array can not be interpreted as valid tag data, corresponding to
> 1163:      *         the {@code tagSignature}
> 1164:      * @throws IllegalArgumentException if this is a profile for one of the

`IllegalStateException` better describes the reason: the argument to the method can be perfectly valid, but the internal state of the object doesn't allow modifications.

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

> 1170:     public void setData(int tagSignature, byte[] tagData) {
> 1171:         if (isBuiltIn) {
> 1172:             throw new IllegalArgumentException("BuiltIn profile"

Suggestion:

            throw new IllegalArgumentException("Built-in profile"

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

PR Review: https://git.openjdk.org/jdk/pull/23606#pullrequestreview-2630072442
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963699390
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963690614
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963694920
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1963695795