BRs,
Lin
*From:*Yasumasa Suenaga <yasue...@gmail.com>
*Sent:* Thursday, February 28, 2019 8:39 AM
*To:* 臧琳<zangl...@jd.com>
*Cc:* David Holmes <david.hol...@oracle.com>;
serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net <serviceability-dev@openjdk.java.net>
*Subject:* Re: Protocol version of Attach API
2019年2月28日(木) 0:04 臧琳 <zangl...@jd.com <mailto:zangl...@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 <yasue...@gmail.com
<mailto:yasue...@gmail.com>>
Sent: Wednesday, February 27, 2019 8:10:14 PM
To: 臧琳
Cc: David Holmes; serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@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 <yasue...@gmail.com
<mailto:yasue...@gmail.com>>
> Sent: Wednesday, February 27, 2019 15:15
> To: David Holmes; 臧琳
> Cc: serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@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.hol...@oracle.com
<mailto:david.hol...@oracle.com>>; Yasumasa Suenaga
>>>>> <yasue...@gmail.com <mailto:yasue...@gmail.com>>
>>>>> Cc: serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@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.hol...@oracle.com
<mailto:david.hol...@oracle.com>>
>>>>>> Sent: Wednesday, February 27, 2019 1:44 PM
>>>>>> To: Yasumasa Suenaga <yasue...@gmail.com
<mailto:yasue...@gmail.com>>; 臧琳 <zangl...@jd.com
<mailto:zangl...@jd.com>>
>>>>>> Cc: serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@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, zangl...@jd.com
<mailto:zangl...@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
>>>>>>>>
