RFR: 8049695: nsk/jdb/options/connect/connect003 fails with "Launched jdb could not attach to debuggee during 300000 milliseconds"

Fri Mar 23 19:12:08 UTC 2018

Hi Alex,

It looks good to me.
Thank you for the update!

Thanks,
Serguei

On 3/22/18 16:28, Alex Menkov wrote:
> Hi David,
>
> With too-long shmem name java reports:
> ERROR: transport error 202: failed to create shared memory listener: 
> Error: address strings longer than 50 characters are invalid
> and ret.code is 2
>
> I added checks for both ret.code and presence of "address strings 
> longer than" text in the output.
> webrev: 
> http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open.06/
>
> --alex
>
> On 03/22/2018 14:32, David Holmes wrote:
>> On 23/03/2018 6:43 AM, Alex Menkov wrote:
>>>
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open.05/
>>>
>>> The test was updated to ensure shmem name longer than 49 symbols 
>>> causes java failure.
>>
>> This doesn't ensure it failed gracefully:
>>
>> 81         // extra test: ensure using of too-long name fails gracefully
>> 82         // (shmemName + "X") is expected to be "too long".
>> 83         ProcessTools.executeProcess(getTarget(shmemName + "X"))
>> 84                 .shouldNotHaveExitValue(0);
>>
>> It may have crashed. What exactly is the failure mode? return code 1? 
>> Exception message that we can check for in outputAnalyzer ?
>>
>> David
>>
>>> --alex
>>>
>>>
>>> On 03/21/2018 15:39, David Holmes wrote:
>>>> On 22/03/2018 2:41 AM, Alex Menkov wrote:
>>>>> Hi David,
>>>>>
>>>>> On 03/20/2018 21:51, David Holmes wrote:
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 21/03/2018 3:25 AM, Alex Menkov wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> On 03/19/2018 18:10, David Holmes wrote:
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>> On 20/03/2018 10:28 AM, Alex Menkov wrote:
>>>>>>>>> Hi guys,
>>>>>>>>>
>>>>>>>>> please re-review the fix.
>>>>>>>>
>>>>>>>> I still have an unanswered question about where the max of 49 
>>>>>>>> is enforced. I see it for the "address" but not names in 
>>>>>>>> general. ??
>>>>>>>
>>>>>>> for shmem the "channel name" is the address (it's checked in 
>>>>>>> createTransport/openTransport).
>>>>>>> Names for mutexes/events are generated by appending some strings 
>>>>>>> to the adddress and length of the added parts are supposed to be 
>>>>>>> less than MAX_IPC_SUFFIX (25 symbols):
>>>>>>> ".mutex" (+ up to 3 symbols)
>>>>>>> ".hasData" (+ up to 3 symbols)
>>>>>>> ".hasSpace" (+ up to 3 symbols)
>>>>>>> ".ctos"
>>>>>>> ".stoc"
>>>>>>> ".accept" (+ up to 3 symbols)
>>>>>>> ".attach" (+ up to 3 symbols)
>>>>>>> ".<pid>" (pid is a DWORD)
>>>>>>
>>>>>> Okay so ... the code in shmemBase.c is very unclear as to which 
>>>>>> "names" can come in from an external source and which are only 
>>>>>> ever derived from other "names". If the "address" (which seems a 
>>>>>> very bad description in this case!) is the only external source 
>>>>>> for a name, and it is limited to a length of 49 then that is okay.
>>>>>
>>>>> Yes, the "address" is the only external arg, all other names are 
>>>>> constructed from it.
>>>>> I believe it's "address" because it comes from "address" parameter:
>>>>> -Xrunjdwp:transport=st_shmem,address=<shmem_name>
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>> Reg.test is added the the issue.
>>>>>>>>
>>>>>>>> I don't quite follow the test. I see you try to set the name 
>>>>>>>> with a value that is too long, and if that doesn't cause an 
>>>>>>>> overflow and we don't crash that is good. But I'd expect you to 
>>>>>>>> read back the name and check it matches the truncated name with 
>>>>>>>> 49 characters.
>>>>>>>
>>>>>>> The test specifies the maximum length supported (49 symbols)
>>>>>>> (if longer name is specified, "address strings longer than 50 
>>>>>>> characters are invalid" error reported).
>>>>>>
>>>>>> I missed the substring that simply causes the name to be the 
>>>>>> maximum supported length. That would trigger the overflow and so 
>>>>>> suffices as a regression test for this fix.
>>>>>>
>>>>>> Is there another test that already passes a too-long name and 
>>>>>> verifies the error gets thrown?
>>>>>
>>>>> Do you mean name >= 50 symbols?
>>>>> No, there is no such test.
>>>>> I don't think it make much sense (test an arbitrary 
>>>>> implementation-specific restriction), but I can add the case to 
>>>>> the test.
>>>>
>>>> It ensures that using a too-long name fails gracefully.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> --alex
>>>>>
>>>>>>
>>>>>>> As far as I see there is no way to read back the name used to 
>>>>>>> create the transport.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> --alex
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> webrev: 
>>>>>>>>> http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open.04/ 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --alex
>>>>>>>>>
>>>>>>>>> On 03/13/2018 16:14, Alex Menkov wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Please review a small fix for
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8049695
>>>>>>>>>> webrev: 
>>>>>>>>>> http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open/
>>>>>>>>>>
>>>>>>>>>> Root cause of the issue is jbd hungs as a result of the 
>>>>>>>>>> buffer overflow.
>>>>>>>>>>
>>>>>>>>>> In the beginning of the shmemBase.c:
>>>>>>>>>>
>>>>>>>>>> #define MAX_IPC_PREFIX 50   /* user-specified or generated 
>>>>>>>>>> name for */
>>>>>>>>>>                              /* shared memory seg and prefix 
>>>>>>>>>> for other IPC */
>>>>>>>>>> #define MAX_IPC_SUFFIX 25   /* suffix to shmem name for other 
>>>>>>>>>> IPC names */
>>>>>>>>>> #define MAX_IPC_NAME   (MAX_IPC_PREFIX + MAX_IPC_SUFFIX)
>>>>>>>>>>
>>>>>>>>>> buffer (char prefix[]) in function createStream is used to 
>>>>>>>>>> generate base name for mutex/events, so MAX_IPC_PREFIX is not 
>>>>>>>>>> big enough.
>>>>>>>>>>
>>>>>>>>>> --alex