On Thu, 10 Apr 2025 04:15:31 GMT, Harshitha Onkar <hon...@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
