[9] RFR of 8143394: PushbackReader throws NullPointerException

[Daniel Fuchs]

Hi Brian,

The fix looks good to me! Good that you were able to find these
other classes where the same issue appeared.
I see that FilterReader.close() calls in.close(). Hopefully wrapping
this call (from PushbackReader) in a synchronized(lock) block
won't be an issue. I wonder if:

  248     public void close() throws IOException {
249 synchronized (lock) {
  250             buf = null;
  251         }
252 super.close(); 253 }

would be a safer implementation. I don't know the java.io code
in depth - so that's just a question...

The only other possible issue I spotted is in the test:

Because readers[0] is set by one thread and read by both then using
an array without synchronization is not thread safe. I would
advise using an AtomicReference instead.

Best regards,

-- daniel

On 12/4/15 1:23 AM, Brian Burkhalter wrote:
> Please review at your convenience.
>
> Issue:	https://bugs.openjdk.java.net/browse/JDK-8143394
> Patch:	http://cr.openjdk.java.net/~bpb/8143394/webrev.00/
>
> The NPE apparently occurred in PushbackReader because a call to close() was made after the conditional at line 72 was evaluated to ‘false’ but before the variable ‘buf’ was dereferenced at line 87. The fix is to change close() to use the ‘lock’ variable as in the other methods. Similar problems were found to exist in CharArrayReader and StringReader. The test reliably reproduces the NPE in all three classes with the NPE in PushbackReader being reproduced frequently and the NPE in the other classes less so.
>
> Thanks,
>
> Brian