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

[serguei.spitsyn at oracle.com]

On 3/14/18 17:44, Alex Menkov wrote:
> Hi Serguei,
>
> On 03/14/2018 17:33, serguei.spitsyn at oracle.com wrote:
>> Hi Alex,
>>
>> Sorry to being late to this party.
>> Thank you for getting to the root cause of this issue and for extra 
>> updates.
>> All such issues are important for test stabilization, so now there 
>> will be one problem less!
>>
>> This looks pretty good to me.
>> I'd replace "objectName" with "streamName" to keep it unified.
>
> In createStream() and createTransport() the buffer is used to generate 
> names for mutex and events (kind of "Windows objects").
> In createConnection() the buffer is used to generate stream names 
> (client->server & server->client streams).

Right, I missed this.
Sorry for the noise. :)

Thanks,
Serguei


>
> --alex
>
>> But I understand why you are trying to avoid using "streamName" in 
>> this particular case.
>> It is because we already have the argument "name" for the stream, so 
>> there can be
>> a confusion why do we have also "streamName" as the argument already 
>> took this role.
>> A better name for argument would be "baseName" (or "prefix") to avoid 
>> this confusion.
>> But I think, this confusion is not that big, so the "streamName" 
>> should be fine.
>>
>> I leave it up to you and other reviewers.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 3/14/18 16:28, Alex Menkov wrote:
>>> Hi Chris,
>>>
>>> On 03/14/2018 13:42, Chris Plummer wrote:
>>>> Hi Alex,
>>>>
>>>> I don't think prefix -> basename is what David had in mind. Those 
>>>> basically mean the same thing. The buffer is being used for the 
>>>> full name, which is why neither is really appropriate. So maybe 
>>>> just call it fullname, or even just name. createConnection() has a 
>>>> similar prefix reference that should be fixed.
>>>
>>> Ok, I don't like "fullname", "name" is already used there, so I made 
>>> them "objectName" (for mutex/event names) and streamName (for stream 
>>> name in createConnection()).
>>>
>>> updated webrev:
>>> http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open.03/
>>>
>>> --alex
>>>
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 3/14/18 12:43 PM, Alex Menkov wrote:
>>>>>
>>>>> Updated fix:
>>>>> http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open.02/
>>>>>
>>>>> The changes:
>>>>> - createTransport function is fixed;
>>>>> - "prefix" variable is renamed to "baseName".
>>>>>
>>>>> --alex
>>>>>
>>>>> On 03/14/2018 09:45, Alex Menkov wrote:
>>>>>> Hi David,
>>>>>>
>>>>>>
>>>>>> On 03/13/2018 17:46, David Holmes wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On 14/03/2018 9:14 AM, 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.
>>>>>>>
>>>>>>> Good catch! But overall this code seems to be missing bounds 
>>>>>>> checks everywhere. You made the "prefix" (poor name?) buffer 
>>>>>>> bigger (MAX_IPC_NAME) but do we know the incoming name plus the 
>>>>>>> appended descriptive string will fit in it?
>>>>>>
>>>>>> Yes, the possible values can be added to the shmem name (which is 
>>>>>> restricted by 49 chars):
>>>>>> ".mutex"
>>>>>> ".hasData"
>>>>>> ".hasSpace"
>>>>>> ".accept"
>>>>>> ".attach"
>>>>>> ".<pid>" (pid is 64bit value, max len IIRC is 19 symbols)
>>>>>> So extra MAX_IPC_SUFFIX (25 symbols) is enough
>>>>>>
>>>>>>> Looking at createTransport for example, it also has:
>>>>>>>
>>>>>>> char prefix[MAX_IPC_PREFIX];
>>>>>>>
>>>>>>> and it produces an error if
>>>>>>>
>>>>>>> strlen(address) >= MAX_IPC_PREFIX
>>>>>>>
>>>>>>> but otherwise copies it across:
>>>>>>>
>>>>>>> strcpy(transport->name, address);
>>>>>>>
>>>>>>> and then later does:
>>>>>>>
>>>>>>>   sprintf(prefix, "%s.mutex", transport->name);
>>>>>>>
>>>>>>> so we may have overflowed again by adding ".mutex"! The same 
>>>>>>> goes for the subsequent sprintf's.
>>>>>>
>>>>>> Thank you for the catch!
>>>>>> I looked the file for other similar issues, but somehow 
>>>>>> overlokked this case.
>>>>>> Will fix it.
>>>>>> Also will change confusing "prefix" name to "base_name".
>>>>>>
>>>>>> --alex
>>>>>>
>>>>>>>
>>>>>>> So I think there is more work to do to ensure this code is 
>>>>>>> immune from buffer overflows.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> --alex
>>>>
>>>>
>>>>
>>