Re: RFR 8240902: JDI shared memory connector can use already closed Handles

Wed, 18 Mar 2020 14:04:38 -0700

Hi Patricio,

Good finding, thank you for taking care about this!
The fix looks good in general.

There are several spots with the wrong indent (must be 4, not 2):
  64 #define ENTER_CONNECTION(connection) do { \
  65                                           InterlockedIncrement(&connection->refcount); \
  66                                           if (IS_STATE_CLOSED(connection->state)) { \
  67                                             setLastErrorMsg("stream closed"); \
  68                                             InterlockedDecrement(&connection->refcount); \
  69                                             return SYS_ERR; \
  70                                           } \
  71                                         } while (0)
  72 
  73 #define LEAVE_CONNECTION(connection) do { \
  74                                           InterlockedDecrement(&connection->refcount); \
  75                                         } while (0)
  I'd also suggest to move content left and use indent 4 from the side.

 414         if (*refcount == 0) {
 415           sysEventClose(stream->hasData);
 416           sysEventClose(stream->hasSpace);
 417           sysIPMutexClose(stream->mutex);
 418           break;
 419         }
 ...

 535     Stream * stream = &connection->outgoing;
 536     if (stream->state == STATE_OPEN) {
 537       (void)closeStream(stream, JNI_TRUE, &connection->refcount);
 538     }
 539     stream = &connection->incoming;
 540     if (stream->state == STATE_OPEN) {
 541       (void)closeStream(stream, JNI_FALSE, &connection->refcount);
 542     }
 ...

 551       if (connection->shutdown) {
 552         sysEventClose(connection->shutdown);
 553       }
 554     }
 ...

1022 shmemBase_sendByte(SharedMemoryConnection *connection, jbyte data)
1023 {
1024   ENTER_CONNECTION(connection);
1025   jint rc = shmemBase_sendByte_internal(connection, data);
1026   LEAVE_CONNECTION(connection);
1027   return rc;
1028 }
 ...

1055 jint
1056 shmemBase_receiveByte(SharedMemoryConnection *connection, jbyte *data)
1057 {
1058   ENTER_CONNECTION(connection);
1059   jint rc = shmemBase_receiveByte_internal(connection, data);
1060   LEAVE_CONNECTION(connection);
1061   return rc;
1062 }
...

1136 jint
1137 shmemBase_sendPacket(SharedMemoryConnection *connection, const jdwpPacket *packet)
1138 {
1139   ENTER_CONNECTION(connection);
1140   jint rc = shmemBase_sendPacket_internal(connection, packet);
1141   LEAVE_CONNECTION(connection);
1142   return rc;
1143 }
...
1229 jint
1230 shmemBase_receivePacket(SharedMemoryConnection *connection, jdwpPacket *packet)
1231 {
1232   ENTER_CONNECTION(connection);
1233   jint rc = shmemBase_receivePacket_internal(connection, packet);
1234   LEAVE_CONNECTION(connection);
1235   return rc;
1236 }

Some other nits were already commented by David and Dan.

I'd suggest to test with tier-5 as well for more safety.

Thanks,
Serguei


On 3/17/20 13:14, Patricio Chilano wrote:
Hi all,

Please review the following patch:

Bug: https://bugs.openjdk.java.net/browse/JDK-8240902
Webrev: http://cr.openjdk.java.net/~pchilanomate/8240902/v1/webrev/

Calling closeConnection() on an already created/opened connection includes calls to CloseHandle() on objects that can still be used by other threads. This can lead to either undefined behavior or, as detailed in the bug comments, changes of state of unrelated objects. This issue was found while debugging the reason behind some jshell test failures seen after pushing 8230594. Not as important, but there are also calls to closeStream() from createStream()/openStream() when failing to create/open a stream that will return after executing "CHECK_ERROR(enterMutex(stream, NULL));" without closing the intended resources. Then, calling closeConnection() could assert if the reason of the previous failure was that the stream's mutex failed to be created/opened. These patch aims to address these issues too.

Tested in mach5 with the current baseline, tiers1-3 and several runs of open/test/langtools/:tier1 which includes the jshell tests where this connector is used. I also applied patch http://cr.openjdk.java.net/~pchilanomate/8240902/triggerbug/webrev mentioned in the comments of the bug, on top of the baseline and run the langtool tests with and without this fix. Without the fix running around 30 repetitions already shows failures in tests jdk/jshell/FailOverExecutionControlTest.java and jdk/jshell/FailOverExecutionControlHangingLaunchTest.java. With the fix I run several hundred runs and saw no failures. Let me know if there is any additional testing I should do.

As a side note, I see there are a couple of open issues related with jshell failures (8209848) which could be related to this bug and therefore might be fixed by this patch.

Thanks,
Patricio


Reply via email to