A Bug about the JVM Attach mechanism

[Yasumasa Suenaga]

Hi Serguei,

Thank you for your comment.
I uploaded new webrev which includes the fix.

  http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.3/

Also I added TODO for other Un*x OSes.
If the change for Linux is OK, I will copy it to others.
(I do not have them, so I depend on submit repo.)

This webrev has passed test/hotspot/jtreg/serviceability/attach and
test/jdk/com/sun/tools/attach jtreg tests on Linux x64.


Thanks,

Yasumasa

2019年6月27日(木) 6:10 serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>:
>
> Hi Yasumasa,
>
> I'm reviewing it.
>
> Just a quick comment.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/src/hotspot/os/linux/attachListener_linux.cpp.udiff.html
>
> +  // reads a request from the given connected socket
> +  static LinuxAttachOperation* read_request(int s);
> +
> +  static bool _atexit_registered;
> +
> + public:
> +  enum {
> +    ATTACH_PROTOCOL_VER = 1                     // protocol version
> +  };
> +  enum {
> +    ATTACH_ERROR_BADVERSION     = 101           // error codes
> +  };
> +
>    static void set_path(char* path) {
>      if (path == NULL) {
> +      _path[0] = '\0';
>        _has_path = false;
>      } else {
>        strncpy(_path, path, UNIX_PATH_MAX);
>        _path[UNIX_PATH_MAX-1] = '\0';
>        _has_path = true;
>      }
>    }
>
>    static void set_listener(int s)               { _listener = s; }
>
> -  // reads a request from the given connected socket
> -  static LinuxAttachOperation* read_request(int s);
> -
> - public:
> -  enum {
> -    ATTACH_PROTOCOL_VER = 1                     // protocol version
> -  };
> -  enum {
> -    ATTACH_ERROR_BADVERSION     = 101           // error codes
> -  };
> -
>
>
> You moved public definitions of enums up in the code.
> All the declarations below that were private before became public now.
> Not sure, if it was your intention
>
> Also, the Copyright comment in this file needs an update.
>
> Thanks,
> Serguei
>
>
>
> On 6/26/19 07:34, Yasumasa Suenaga wrote:
>
> Hi,
>
> >> I'm not clear how this addresses the issue with deleting the file? The
> >> file still has to exist IIUC for the mechanism to work.
> >>
> >> This seems more suited to fixing the "multiple attach threads" problem.
>
> How about this change?
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/
>
> I think it can fix both JDK-8225690 and JDK-8225193.
>
> This webrev implements for Linux only.
> If we work further based on this, we need to implement AttachListener::check_socket_file() for each OS's implementation.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2019/06/21 23:16, Yasumasa Suenaga wrote:
>
> On 2019/06/21 22:51, David Holmes wrote:
>
> Hi Yasumasa,
>
> On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
>
> Hi,
>
> Can we fix this issue like this webrev?
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/
>
>
> I'm not clear how this addresses the issue with deleting the file? The file still has to exist IIUC for the mechanism to work.
>
> This seems more suited to fixing the "multiple attach threads" problem.
>
>
> Yes, my proposal focuses to "multiple attach threads".
> I shared my patch because it might help the work for this issue.
>
> This patch would guard multithread-issue in Attach Listener.
> So unix domain socket file will create just once.
>
>
> Yasumasa
>
>
> David
>
> I could reproduce this issue with ConcAttachTest.java in it
> on my laptop.
> (Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)
>
> If we need to fix based on current implementation, I think
> we need to use atomic operation.
> But if we can refactor around here, we might be able to another approach - e.g. using monitors/mutexes
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2019/06/21 5:49, David Holmes wrote:
>
> Sorry it took me a while to understand the specifics of the problem. :)
>
> David
>
> On 20/06/2019 3:37 am, nijiaben wrote:
>
> Yes Alan, I mean this
> ------------------?Original?------------------
> *From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
> *Date: *?Thu, Jun 20, 2019 02:54 PM
> *To: *?"nijiaben"<nijiaben at perfma.com>; "David
> Holmes"<david.holmes at oracle.com>;
> "serviceability-dev"<serviceability-dev at openjdk.java.net>;
> "jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
> "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
> *Subject: *?Re: A Bug about the JVM Attach mechanism
> On 20/06/2019 05:10, nijiaben wrote:
>   > :
>   > I know this mechanism, can we provide means of recovery to avoid
> unavailability caused by accidental deletion?
>   >
> Are you concerned about tmpreaper or cron jobs that periodically cleanup
> /tmp? There may indeed be an issue for applications that run for weeks
> or months. If someone is using jmap, jcmd or other tools using the
> attach API then it will trigger the attach listener to start. When they
> come back in a few weeks then the .java_pid<pid> file may have been
> removed so they cannot attach. Is this what what you are pointing out?
>
> -Alan
>
>