On 3/14/18 17:44, Alex Menkov wrote:
Hi Serguei,
On 03/14/2018 17:33, serguei.spit...@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
