kohlschutter / junixsocket

Unix Domain Sockets in Java 7 and newer (AF_UNIX), AF_TIPC, AF_VSOCK, and more
Apache License 2.0
433 stars 114 forks source link

Patch from docker-java for reading from closed socket #31

Closed CodingFabian closed 4 years ago

CodingFabian commented 7 years ago

In https://github.com/docker-java/docker-java/pull/697 they monkey patched junixsocket in org.newsclub.net.unix.AFUNIXSocketImpl.AFUNIXInputStream.read(byte[], int, int)

      if (closed) {
        return -1;
      }

I wonder if thats a valid change? I observed at a customer a thread hanging in a socket read at exactly this place:

  Thread "scheduler-thread-4":
    at org.newsclub.net.unix.NativeUnixSocket.read(java.io.FileDescriptor, byte[ ], int, int)
    at org.newsclub.net.unix.AFUNIXSocketImpl$AFUNIXInputStream.read(byte[ ], int, int) (line: 189)
    at sun.nio.cs.StreamDecoder.readBytes() (line: 284)
    at sun.nio.cs.StreamDecoder.implRead(char[ ], int, int) (line: 326)
    at sun.nio.cs.StreamDecoder.read(char[ ], int, int) (line: 178)
    at java.io.InputStreamReader.read(char[ ], int, int) (line: 184)
    at java.io.BufferedReader.fill() (line: 161)
    at java.io.BufferedReader.readLine(boolean) (line: 324)
    at java.io.BufferedReader.readLine() (line: 389)

maybe thats a potential fix?

kohlschuetter commented 5 years ago

This could be fixed in 2.1.0. Please verify. Thanks for reporting!

CodingFabian commented 5 years ago

thanks for working on this. Looks like you throw an exception instead of returning -1? Not sure which one is actually correct.

Javadoc of read says: the total number of bytes read into the buffer, or -1 if there is no more data because the end of the stream has been reached.

so as long as you differentiate premature closing (which warrants an exception) or just trying to read from the end of the stream, this looks good.

Will try the new version soon and come back.

dbyron0 commented 5 years ago

From what I can see there's no change from 2.0.4 (which docker-java uses) to 2.1.2. AFUNIXInputStream threw an exception on streamClosed, and still does in 2.1.2. There's still no check to return -1 if closed in 2.1.2.

kohlschuetter commented 5 years ago

Shouldn't EndOfFileTest#clientReadEof catch this case? If not, can you repro the issue with 2.1.2, and perhaps write a test case? Cheers!

dbyron0 commented 5 years ago

Struggling to get the tests on master to pass locally, so trying https://github.com/kohlschutter/junixsocket/pull/56.

kohlschuetter commented 5 years ago

The behavior of throwing a SocketException upon trying to read from a closed socket is identical to what happens with vanilla (TCP / java.net) Sockets.

Try copying the two methods into EndOfFileJavaTest, and see that the tests also fail without junixsocket's involvement.

If Docker indeed expects -1 despite violating the java.net API, an easy workaround would be to catch all IOExceptions upon read, checking whether the socket#isClosed, and returning -1 in this case instead.

See the CloseIgnorantInputStream.java for a potential solution that doesn't require patching junixsocket.