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