Protocol version of Attach API

David Holmes david.holmes at oracle.com
Thu Feb 28 04:59:28 UTC 2019


Sorry I'm going to pick up on the rollback and re-do option here as I 
just had a closer look at jmap. Given jmap -dump already has more 
options than -histo does, why was any change to the "maximum number of 
args" needed in the first place ???

David

On 28/02/2019 2:43 pm, David Holmes wrote:
> Hi everyone,
> 
> I'm not sure we're converging on a suitable solution here, but to 
> address the issues flagged by Lin below ...
> 
> On 28/02/2019 12:39 pm, 臧琳 wrote:
>> Hi Suenaga,
>>
>>       Thanks for your expaination about  the arg_length_max, I 
>> generally agree with you that it is better to consider using dynamic 
>> memory, and that would be handled carefully to aviod introduce 
>> compatibility issue, plus it would be a big change. So let’s see what 
>> others suggest.
>>
>> Hi All,
>>
>> It seems for me that there are basically three problems forked by this 
>> thread:
>>
>> ·Compatibility issue with old jcmd alike tools with attachListener’s 
>> change.
> 
> This is issue:
> 
> https://bugs.openjdk.java.net/browse/JDK-8219721
> 
>> ·Only 3 arguments limited to passed by socket to attachListener for 
>> Windows, which cause 8215622 work abnormally on Windows.
> 
> I have filed a new bug for this:
> 
> https://bugs.openjdk.java.net/browse/JDK-8219895
> 
>> ·The arg_length_max may not be enough for handling filename.
> 
> I have filed a new bug for this:
> 
> https://bugs.openjdk.java.net/browse/JDK-8219896
> 
> though it seems very related to this issue.
> 
>> So I suggest we keep the first problem discussed in this thread, and 
>> create separate thread for the others. Do you agree?
> 
> There is some overlap but yes this can be broken down somewhat - though 
> dealing with the variable length "packet" is going to have to consider 
> that what is received is in fact much larger than the purported maximum 
> packet size if these long paths are expected and accepted.
> 
> FWIW I don't see crashes or anything drastic if the arguments are too 
> long - the operations just fail (in somewhat obscure ways sometimes).
> 
>>
>> For me, I will refine my patch to use timeout as a fix for the first 
>> one, and update it in this thread. And I will try to fix the second 
>> one for Windows, and create a separate thread for discussing. And if 
>> possible, I can help to fix the third one.
>>
>> What’s your opinion?
> 
> That sounds fine ...
> 
> Or you could choose to rollback JDK-8215622 and see how to solve that 
> without increasing the arg count. Given this usage:
> 
> jmap -histo:live,file=foo.txt <pid>
> 
> I'm not sure why this is sent to the VM as multiple args rather than as 
> a single composite arg that can then be parsed again by the actual 
> "jmap" logic. There would be some double-up perhaps if the front-end 
> tool wants to perform the command-line validation, but it would be easy 
> enough I think to do that checking then send the original composite arg.
> 
> Thanks,
> David
> -----
> 
> 
>> BRs,
>>
>> Lin
>>
>> *From:*Yasumasa Suenaga <yasuenag at gmail.com>
>> *Sent:* Thursday, February 28, 2019 8:39 AM
>> *To:* 臧琳<zanglin5 at jd.com>
>> *Cc:* David Holmes <david.holmes at oracle.com>; 
>> serviceability-dev at openjdk.java.net 
>> serviceability-dev at openjdk.java.net <serviceability-dev at openjdk.java.net>
>> *Subject:* Re: Protocol version of Attach API
>>
>> 2019年2月28日(木) 0:04 臧琳 <zanglin5 at jd.com <mailto:zanglin5 at jd.com>>:
>>
>>     Dear Suenaga,
>>            Thanks for your reviewing. I will try to refine the patch.
>>            For the argument length you mentioned, do you mean the
>>     "arg_length_max" should be large enough to accept the max filename
>>     length?
>>
>> Yes, but it is not enough.
>>
>> For example, jcmd VM.log might pass 2 or more paths to define logs.
>>
>>            IMHO, all the handling of the argument length is at receiver
>>     side in the attachListener, such as
>>     
>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/hotspot/os/linux/attachListener_linux.cpp#l322, 
>>     for me it means that the VM side limits the argments length less
>>     than arg_length_max, otherwise it will return NULL, which cause the
>>     sender side (tools like jcmd and jmap) exits with error message. so
>>     I think there may be no need to limit the argument size in tool side.
>>
>> IMHO all programs which use filesystem should support any locations on 
>> it.
>>
>> So I think we should use dynamic memory (or GrowableArray) for it if 
>> we do not change client side for compatibility.
>>
>>            And from my experiment with jmap, the arguments send to
>>     sockets are not arg0 only.  as you can see in
>>     
>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java#l193 
>>
>>     and
>>     
>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java#l133, 
>>
>>     jmap can pass arg0 as "filename", and arg1 as "-live", and both of
>>     them can be NULL. so <ver>0<cmd>0<arg>0<arg>0<arg>0  can be
>>     <ver>0<jmap>0<filename>0<live>0, and file can be null. so 00 may not
>>     indicate it reach the end.
>>
>> We should consider for other tools - jstack and jinfo.
>>
>> (jstack is ok because it will not have long arguments)
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>     BRs,
>>     Lin
>>     ________________________________________
>>     From: Yasumasa Suenaga <yasuenag at gmail.com 
>> <mailto:yasuenag at gmail.com>>
>>     Sent: Wednesday, February 27, 2019 8:10:14 PM
>>     To: 臧琳
>>     Cc: David Holmes; serviceability-dev at openjdk.java.net
>>     <mailto:serviceability-dev at openjdk.java.net>
>>     Subject: Re: Protocol version of Attach API
>>
>>     Hi Lin,
>>
>>     I think we need to research more about this.
>>     IMHO we need to match length of arguments between
>>     server (AttachListener) and client (serviceability tools) at least.
>>     (please see previous email from me).
>>
>>     I have some comments for your change:
>>
>>     On 2019/02/27 18:22, 臧琳 wrote:
>>      > Dear All,
>>      >      Here I have figured out one solution based on timeout. would
>>     you like help to see whether this is OK?
>>      > --- a/src/hotspot/os/linux/attachListener_linux.cpp     Tue Feb
>>     26 14:57:23 2019 +0530
>>      > +++ b/src/hotspot/os/linux/attachListener_linux.cpp     Wed Feb
>>     27 17:21:48 2019 +0800
>>      > @@ -263,9 +263,29 @@
>>      >     int off = 0;
>>      >     int left = max_len;
>>      >
>>      > +  memset(buf, 0, max_len);
>>      > +  // set timeout for read
>>      > +  struct timeval timeout;
>>      > +  timeout.tv_sec = 3;
>>      > +  timeout.tv_usec = 0;
>>
>>     I think timeout value should be configurable.
>>     For example, we can introduce new flag in globals.hpp .
>>
>>
>>      > +  if(setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, (struct
>>     timeval*)&timeout, sizeof(timeout))) {
>>      > +    log_debug(attach)("Failed to set socket option SO_RCVTIMEO:
>>     %s\n", strerror(errorno));
>>      > +    printf("Warning: Failed to set socket option SO_RCVTIMEO:
>>     %s!!!\n", strerror(errno));
>>
>>     You should not use printf(), and do you need to pass '\n' to
>>     log_debug()?
>>
>>
>>      > +  }
>>      >     do {
>>      >       int n;
>>      > -    RESTARTABLE(read(s, buf+off, left), n);
>>      > +    log_debug(attach)("start reading data from socket....\n");
>>      > +    n = read(s, buf+off, left);
>>
>>     You should use RESTARTABLE macro.
>>     read(2) might be interrupted by signal.
>>
>>
>>      > +    if (n < 0) {
>>      > +       if (errno == EWOULDBLOCK) {
>>
>>     According to man page, read(2) sets EWOULDBLOCK or EAGAIN.
>>     So you should check both errno.
>>
>>
>>      > +         for (int i = str_count; i < expected_str_count; i++) {
>>      > +           //timeout, fill reminded arguments with \0;
>>      > +           buf[off+i] = '\0';
>>      > +           str_count++;
>>      > +         }
>>
>>     You set zero to buf[] in above.
>>     So you can remove this loop, and set str_count to expected_str_count
>>     without manipulating buf[].
>>
>>     In addition, I prefer to add log_debug() at this
>>     to record NULL arguments are added.
>>
>>
>>      > +         break;;
>>      > +       }
>>      > +    }
>>      >       assert(n <= left, "buffer was too small, impossible!");
>>      >       buf[max_len - 1] = '\0';
>>      >       if (n == -1) {
>>
>>
>>     Thanks,
>>
>>     Yasumasa
>>
>>
>>      > Thanks.
>>      > Lin
>>      >
>>      > ________________________________________
>>      > From: Yasumasa Suenaga <yasuenag at gmail.com
>>     <mailto:yasuenag at gmail.com>>
>>      > Sent: Wednesday, February 27, 2019 15:15
>>      > To: David Holmes; 臧琳
>>      > Cc: serviceability-dev at openjdk.java.net
>>     <mailto:serviceability-dev at openjdk.java.net>
>>      > Subject: Re: Protocol version of Attach API
>>      >
>>      > On 2019/02/27 15:59, David Holmes wrote:
>>      >> On 27/02/2019 4:10 pm, Yasumasa Suenaga wrote:
>>      >>> Hi,
>>      >>>
>>      >>> Buffer size for receiving packets from client is determined 
>> at [1].
>>      >>
>>      >> Maximum buffer size, yes.
>>      >>
>>      >>> It defines length of command name and of argument.
>>      >>> It is passed via Unix domain, so we fill '\0' to remaining
>>     bytes and
>>      >>> might be able to assume all arguments are passed with empty 
>> string.
>>      >>
>>      >> Not sure what you mean.
>>      >>
>>      >> // The buffer is expected to be formatted as follows:
>>      >> // <ver>0<cmd>0<arg>0<arg>0<arg>0
>>      >>
>>      >> so we can expect to read at least two things - the ver and cmd.
>>     If we encounter 00 we can infer we reached the end. But we may not
>>     have read the full data into the buffer, so can't tell if another
>>     call to read() is needed yet - you only know after you've read the 
>> 00.
>>      >>
>>      >>> BTW length of arguments is defined to 1024 in [2].
>>      >>> jcmd and jmap might pas file path - it might be JVM_MAXPATHLEN
>>     (4097 bytes).
>>      >>> Buffer size which is used in AttachListener seems not to be 
>> enough.
>>      >>
>>      >> One has to assume/hope that the code sending the data is working
>>     to the same protocol rules as the receiver! Otherwise this is just
>>     completely broken.
>>      >
>>      > On Linux, client (VirtualMachineImpl) seems not to check length
>>     of arguments:
>>      >
>>     
>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java#l168 
>>
>>      >
>>      > In case of jcmd, all options are passed to arg #1. It seems not
>>     to check the length:
>>      >
>>     
>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java#l111 
>>
>>      >
>>      >
>>      > I guess other tools (jstack, jmap, etc) which connect to
>>     AttachListener are same.
>>      > So we can fix both Attach API and AttachListener (it will be big
>>     change!),
>>      > but I concern we can keep protocol version...
>>      >
>>      >
>>      > Thanks,
>>      >
>>      > Yasumasa
>>      >
>>      >
>>      >> David
>>      >> -----
>>      >>
>>      >>> We might have to change more.
>>      >>>
>>      >>>
>>      >>> Thanks,
>>      >>>
>>      >>> Yasumasa
>>      >>>
>>      >>>
>>      >>> [1]
>>     
>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/hotspot/os/linux/attachListener_linux.cpp#l254 
>>
>>      >>> [2]
>>     
>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/hotspot/share/services/attachListener.hpp#l106 
>>
>>      >>>
>>      >>>
>>      >>> On 2019/02/27 15:00, 臧琳 wrote:
>>      >>>> Another solution I can figure out is try to add timeout for
>>     socket read. I will also investigate whether is works.
>>      >>>>
>>      >>>> Cheers,
>>      >>>> Lin
>>      >>>>
>>      >>>>
>>      >>>>> -----Original Message-----
>>      >>>>> From: 臧琳
>>      >>>>> Sent: Wednesday, February 27, 2019 1:52 PM
>>      >>>>> To: 'David Holmes' <david.holmes at oracle.com
>>     <mailto:david.holmes at oracle.com>>; Yasumasa Suenaga
>>      >>>>> <yasuenag at gmail.com <mailto:yasuenag at gmail.com>>
>>      >>>>> Cc: serviceability-dev at openjdk.java.net
>>     <mailto:serviceability-dev at openjdk.java.net>
>>      >>>>> Subject: RE: Protocol version of Attach API
>>      >>>>>
>>      >>>>> Dear David, Yasumasa,
>>      >>>>>         I think it is hard to know how long the buffer is
>>     passed from socket
>>      >>>>> without changing the info of the message from the receiver 
>> side.
>>      >>>>>         So how about when str_count reach to 3, we test it
>>     with non_blocking
>>      >>>>> read?
>>      >>>>>
>>      >>>>>
>>      >>>>> Cheers,
>>      >>>>> Lin
>>      >>>>>> -----Original Message-----
>>      >>>>>> From: David Holmes <david.holmes at oracle.com
>>     <mailto:david.holmes at oracle.com>>
>>      >>>>>> Sent: Wednesday, February 27, 2019 1:44 PM
>>      >>>>>> To: Yasumasa Suenaga <yasuenag at gmail.com
>>     <mailto:yasuenag at gmail.com>>; 臧琳 <zanglin5 at jd.com
>>     <mailto:zanglin5 at jd.com>>
>>      >>>>>> Cc: serviceability-dev at openjdk.java.net
>>     <mailto:serviceability-dev at openjdk.java.net>
>>      >>>>>> Subject: Re: Protocol version of Attach API
>>      >>>>>>
>>      >>>>>> Hi Yasumasa,
>>      >>>>>>
>>      >>>>>> On 27/02/2019 1:05 pm, Yasumasa Suenaga wrote:
>>      >>>>>>> Hi Lin,
>>      >>>>>>>
>>      >>>>>>> My proposal is a just idea, so you need to tweak it.
>>      >>>>>>>
>>      >>>>>>> AttachListener receives raw command.
>>      >>>>>>> For example, jcmd is `jcmd\0<arg strings>`. Please see
>>      >>>>>>> HotSpotVirtualMachine and extended classes.
>>      >>>>>>>
>>      >>>>>>> In case of jcmd, I guess AttachListener will receive message
>>      >>>>>>> `<version>\0jcmd\0<args>\0\0\0` (I did not check it well).
>>      >>>>>>> So I guess we can add '\0' when `str_count` does not reach
>>     to maximum.
>>      >>>>>>
>>      >>>>>> I don't see how this approach can work. You have to know how
>>     many
>>      >>>>>> arguments are coming in the "packet", but that information
>>     is not
>>      >>>>>> available in the current Linux implementation.Without it you
>>     can't
>>      >>>>>> know when to stop calling read().
>>      >>>>>>
>>      >>>>>> The protocol would need to be changed to send the "packet"
>>     size, but
>>      >>>>>> that's not compatible with older JDKs.
>>      >>>>>>
>>      >>>>>> Otherwise I think we have no choice but to use a
>>     non-blocking read ...
>>      >>>>>> though I'm still unsure if you can know for certain that 
>> you've
>>      >>>>>> reached the end of the "packet" ...
>>      >>>>>>
>>      >>>>>> Thanks,
>>      >>>>>> David
>>      >>>>>>
>>      >>>>>>>
>>      >>>>>>> Thanks,
>>      >>>>>>>
>>      >>>>>>> Yasumasa
>>      >>>>>>>
>>      >>>>>>>
>>      >>>>>>> On 2019/02/27 11:50, zanglin5 at jd.com
>>     <mailto:zanglin5 at jd.com> wrote:
>>      >>>>>>>> Dear  Yasumasa,
>>      >>>>>>>>     The fix you mentioned seems not work as expected.
>>      >>>>>>>>     I have done an experiment that use jdk1.8's "jcmd
>>     <pid> help" to
>>      >>>>>>>> attach to latest jdk.
>>      >>>>>>>>     it seem the whole "jcmd <pid> help"  buffer is not
>>      >>>>>>>>     read completely at one time. in my case it is "jcmd",
>>     "<pid>",
>>      >>>>>>>> "help", and then block at while().
>>      >>>>>>>>     After applied your change, it seems only the "jcmd" is
>>      >>>>>>>> processed, the "<pid>", "help" is replaced by '\0'.
>>      >>>>>>>>
>>      >>>>>>>> BRs,
>>      >>>>>>>> Lin
>>      >>>>>>>>
>>


More information about the serviceability-dev mailing list