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

[Patricio Chilano]

Hi Serguei,

On 3/18/20 6:03 PM, serguei.spitsyn at oracle.com wrote:
> 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.
Done. I already aligned it the same way as STREAM_INVARIANT and I fixed 
the indent inside ENTER_CONNECTION().

> 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 }
Done. Fix all those.

> Some other nits were already commented by David and Dan.
>
> I'd suggest to test with tier-5 as well for more safety.
Thanks for looking at this Serguei! I'll give it a new run in mach5 and 
add tier5.


Thanks,
Patricio
> 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
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200318/28289c43/attachment-0001.htm>