Protocol version of Attach API

[David Holmes]

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.

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>; Yasumasa Suenaga
>>> <yasuenag at gmail.com>
>>> Cc: 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>
>>>> Sent: Wednesday, February 27, 2019 1:44 PM
>>>> To: Yasumasa Suenaga <yasuenag at gmail.com>; 臧琳 <zanglin5 at jd.com>
>>>> Cc: 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 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
>>>>>>