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

[Alex Menkov]

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