8367384: The ICC_Profile class may throw exceptions during serialization #27326
Conversation
|
👋 Welcome back serb! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
| case null, default -> getInstance(data); | ||
| }; | ||
| } catch (ClassCastException | IllegalArgumentException e) { | ||
| throw new InvalidObjectException("Invalid ICC Profile Data", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is possibility for optimization here, because we do not actually need to read the "data" if "csName" is a valid constant for the standard profile. However, the spec does not seem to say anything about the optionality of the data field itself, only about the content of the data. So, I left this part unchanged.
| case "CS_LINEAR_RGB" -> getInstance(ColorSpace.CS_LINEAR_RGB); | ||
| case null, default -> getInstance(data); | ||
| }; | ||
| } catch (ClassCastException | IllegalArgumentException e) { |
There was a problem hiding this comment.
I don't see InvalidObjectException as a declared exception either for
This is a little tricky InvalidObjectException isn't listed explicitly by
https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/io/ObjectInputStream.html#readObject()
although it is a subclass of IOException
https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/io/InvalidObjectException.html#%3Cinit%3E(java.lang.String)
However the docs for
https://docs.oracle.com/en/java/javase/25/docs/api/java.desktop/java/awt/color/ICC_Profile.html#getInstance(byte%5B%5D)
say that IOException is thrown by ObjectInputStream
* @throws IOException thrown by {@code ObjectInputStream}
but the exceptions you are catching are from the profile verifier - at least the IAE is.
And there is a (closed) test that expects IAE in this case and fails because it now gets InvalidObjectException
Either that test, or this fix, or both will need revising. Perhaps the spec. of readObject should be updated too.
There was a problem hiding this comment.
I don't see InvalidObjectException as a declared exception either for This is a little tricky InvalidObjectException isn't listed explicitly by https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/io/ObjectInputStream.html#readObject() although it is a subclass of IOException https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/io/InvalidObjectException.html#%3Cinit%3E(java.lang.String)
Throwing "new InvalidObjectException" from readObject for certain issues is a common pattern in the java.base and java.desktop modules. The assumption seems to be that InvalidObjectException is treated as a kind of io error(IOException), since it is usually thrown when the data is “broken” and cannot be saved by the corresponding write method. The specification for "ObjectInputStream.readObject()" could be updated to mention this behavior.
However the docs for https://docs.oracle.com/en/java/javase/25/docs/api/java.desktop/java/awt/color/ICC_Profile.html#getInstance(byte%5B%5D)
say that IOException is thrown by ObjectInputStream * @throws IOException thrown by {@code ObjectInputStream} but the exceptions you are catching are from the profile verifier - at least the IAE is. And there is a (closed) test that expects IAE in this case and fails because it now gets InvalidObjectException
Either that test, or this fix, or both will need revising. Perhaps the spec. of readObject should be updated too.
That closed test should be updated.
Additional checks were recently added to ICC_Profile (see JDK-8347377). As a result, objects previously stored as valid profiles may now throw an IllegalArgumentException during serialization. Discussion about serialization was started in #23044 but it seems at the end non-serialization related check was verified. =(
The patch itself is simple, but I found that we do not have good test coverage in this area. So I added two tests to cover all possible variants specified by the serialization spec.
Progress
Warnings
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27326/head:pull/27326$ git checkout pull/27326Update a local copy of the PR:
$ git checkout pull/27326$ git pull https://git.openjdk.org/jdk.git pull/27326/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27326View PR using the GUI difftool:
$ git pr show -t 27326Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27326.diff
Using Webrev
Link to Webrev Comment