8385017: Null pointer dereference in java.desktop/share/classes/com/sun/media/sound/ModelByteBuffer.java:204#31299
8385017: Null pointer dereference in java.desktop/share/classes/com/sun/media/sound/ModelByteBuffer.java:204#31299prrace wants to merge 2 commits into
Conversation
|
👋 Welcome back prr! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
Bot
changed the title
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
| @@ -201,6 +201,9 @@ public ModelByteBuffer(File file, long offset, long len) { | |||
| public void writeTo(OutputStream out) throws IOException { | |||
| if (root.file != null && root.buffer == null) { | |||
| try (InputStream is = getInputStream()) { | |||
| if (is == null) { | |||
| throw new IOException("null stream"); | |||
There was a problem hiding this comment.
This was the actual complaint
There was a problem hiding this comment.
That getInputStream method is used in a few other places that can also not be ready for null.
It can return null because it catches the IOException and returns null instead. Maybe it would be better to just throw the IOException there, instead of returning null and then checking and throwing?
There was a problem hiding this comment.
But I also noticed this case which I decided to fix too.
However the DataInputStream constructor accepts null .. but the readFully() call will throw an NPE so I had to handle it there since it was not so obvious how to check for null inside the try(..)
I also filed a bug against DataInputStream
https://bugs.openjdk.org/browse/JDK-8385487
Webrevs
|
| @@ -209,8 +210,12 @@ public AudioFloatInputStream openStream() { | |||
| return AudioFloatInputStream.getInputStream(ais); | |||
| } | |||
| if (buffer.array() == null) { | |||
| InputStream is = buffer.getInputStream(); | |||
There was a problem hiding this comment.
this seems to be the same "buffer.getInputStream" use above?
| @@ -209,8 +210,12 @@ public AudioFloatInputStream openStream() { | |||
| return AudioFloatInputStream.getInputStream(ais); | |||
| } | |||
| if (buffer.array() == null) { | |||
| InputStream is = buffer.getInputStream(); | |||
| if (is == null) { | |||
| return null; | |||
There was a problem hiding this comment.
This method is only used in SoftAbstractResampler. And when this one returns null, that will throw NPE:
stream = osc.openStream();
...
samplerateconv = stream.getFormat().getSampleRate() / outputsamplerate;
2 participants
A fix for a theoretical NPE because a method might return null and it is used.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/31299/head:pull/31299$ git checkout pull/31299Update a local copy of the PR:
$ git checkout pull/31299$ git pull https://git.openjdk.org/jdk.git pull/31299/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 31299View PR using the GUI difftool:
$ git pr show -t 31299Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/31299.diff
Using Webrev
Link to Webrev Comment