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

[Daniel D. Daugherty]

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.

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

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

     L412:     MemoryBarrier();     /* Prevent load of refcount to float 
above. */
         typo: s/to float/from floating/

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

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

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


I'm good with the code changes. I only have nits above so I don't need
to see another webrev.

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
>