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

[David Holmes]

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