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