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

[Chris Plummer]

On 3/14/18 4:28 PM, 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/
Hi Alex,

Just noticed the copyright needs updating, but otherwise looks good. No 
need for another webrev.

thanks,

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