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;
+ 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));
+ }
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);
+ if (n < 0) {
+ if (errno == EWOULDBLOCK) {
+ for (int i = str_count; i < expected_str_count; i++) {
+ //timeout, fill reminded arguments with \0;
+ buf[off+i] = '\0';
+ str_count++;
+ }
+ break;;
+ }
+ }
assert(n <= left, "buffer was too small, impossible!");
buf[max_len - 1] = '\0';
if (n == -1) {
Thanks.
Lin
________________________________________
From: Yasumasa Suenaga <[email protected]>
Sent: Wednesday, February 27, 2019 15:15
To: David Holmes; 臧琳
Cc: [email protected]
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' <[email protected]>; Yasumasa Suenaga
>>>> <[email protected]>
>>>> Cc: [email protected]
>>>> 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 <[email protected]>
>>>>> Sent: Wednesday, February 27, 2019 1:44 PM
>>>>> To: Yasumasa Suenaga <[email protected]>; 臧琳 <[email protected]>
>>>>> Cc: [email protected]
>>>>> 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, [email protected] 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
>>>>>>>
