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

[Patricio Chilano]

Hi Dan,

On 3/18/20 5:30 PM, Daniel D. Daugherty wrote:
> On 3/17/20 4:14 PM, 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/
>
> src/jdk.jdi/share/native/libdt_shmem/shmemBase.c
>     L411:     int attempts = 10;
>     L420:         sysSleep(200);
>         I presume that this is a 200 millisecond sleep so this new loop
>         will delay a closeStream() call by at most 2 seconds. You may
>         want those literals to be #define'ed values at the top of the
>         file, e.g., like this one:
>
>         #define MAX_GENERATION_RETRIES 20
>
>         Your choice on the names of the new #defines if you choose to
>         do that. You might even consider putting them close to
>         "typedef struct SharedMemoryConnection".
>
>         Update: Oh yuck! Now I see that there is existing code that
>         does the same kind of looping with sysSleep() calls when the
>         linger option is set. I revise my comment: You're following
>         the existing style in the function so go with what you have.
Ok, I left the loop as it is now.

> Don't forget to update the copyright year before you push.
Done.

> L379: closeStream(Stream *stream, jboolean linger, unsigned int 
> *refcount )
>         nit - please delete space before ')'.
Done.

> L412:     MemoryBarrier();     /* Prevent load of refcount to float 
> above. */
>         typo: s/to float/from floating/
After replying to David's review I realized the enterMutex() call on 
closeStream() will already provide acquire semantics so reading the 
refcount will not float above. I removed the barrier.

> L413:     while (attempts>0) {
>         nit - please add spaces around '>'.
Done.

> L415-418, L537, L541, L552:
>         nit - indent should be four spaces instead of two spaces.
Done.

> The existing L546 and L549 should indented four spaces instead
>         of two spaces. Please fix since you there.
Done.

> I'm good with the code changes. I only have nits above so I don't need
> to see another webrev.
Thanks for reviewing this Dan! I might send a v2 later.


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