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

[Alex Menkov]

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.

--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