+1
On 3/22/18 4:50 PM, David Holmes wrote:
Thanks Alex! Looks good.
David
On 23/03/2018 9:28 AM, Alex Menkov wrote:
Hi David,
With too-long shmem name java reports:
ERROR: transport error 202: failed to create shared memory listener:
Error: address strings longer than 50 characters are invalid
and ret.code is 2
I added checks for both ret.code and presence of "address strings
longer than" text in the output.
webrev:
http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open.06/
--alex
On 03/22/2018 14:32, David Holmes wrote:
On 23/03/2018 6:43 AM, Alex Menkov wrote:
Updated webrev:
http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open.05/
The test was updated to ensure shmem name longer than 49 symbols
causes java failure.
This doesn't ensure it failed gracefully:
81 // extra test: ensure using of too-long name fails
gracefully
82 // (shmemName + "X") is expected to be "too long".
83 ProcessTools.executeProcess(getTarget(shmemName + "X"))
84 .shouldNotHaveExitValue(0);
It may have crashed. What exactly is the failure mode? return code
1? Exception message that we can check for in outputAnalyzer ?
David
--alex
On 03/21/2018 15:39, David Holmes wrote:
On 22/03/2018 2:41 AM, Alex Menkov wrote:
Hi David,
On 03/20/2018 21:51, David Holmes wrote:
Hi Alex,
On 21/03/2018 3:25 AM, Alex Menkov wrote:
Hi David,
On 03/19/2018 18:10, David Holmes wrote:
Hi Alex,
On 20/03/2018 10:28 AM, Alex Menkov wrote:
Hi guys,
please re-review the fix.
I still have an unanswered question about where the max of 49
is enforced. I see it for the "address" but not names in
general. ??
for shmem the "channel name" is the address (it's checked in
createTransport/openTransport).
Names for mutexes/events are generated by appending some
strings to the adddress and length of the added parts are
supposed to be less than MAX_IPC_SUFFIX (25 symbols):
".mutex" (+ up to 3 symbols)
".hasData" (+ up to 3 symbols)
".hasSpace" (+ up to 3 symbols)
".ctos"
".stoc"
".accept" (+ up to 3 symbols)
".attach" (+ up to 3 symbols)
".<pid>" (pid is a DWORD)
Okay so ... the code in shmemBase.c is very unclear as to which
"names" can come in from an external source and which are only
ever derived from other "names". If the "address" (which seems a
very bad description in this case!) is the only external source
for a name, and it is limited to a length of 49 then that is okay.
Yes, the "address" is the only external arg, all other names are
constructed from it.
I believe it's "address" because it comes from "address" parameter:
-Xrunjdwp:transport=st_shmem,address=<shmem_name>
Reg.test is added the the issue.
I don't quite follow the test. I see you try to set the name
with a value that is too long, and if that doesn't cause an
overflow and we don't crash that is good. But I'd expect you
to read back the name and check it matches the truncated name
with 49 characters.
The test specifies the maximum length supported (49 symbols)
(if longer name is specified, "address strings longer than 50
characters are invalid" error reported).
I missed the substring that simply causes the name to be the
maximum supported length. That would trigger the overflow and so
suffices as a regression test for this fix.
Is there another test that already passes a too-long name and
verifies the error gets thrown?
Do you mean name >= 50 symbols?
No, there is no such test.
I don't think it make much sense (test an arbitrary
implementation-specific restriction), but I can add the case to
the test.
It ensures that using a too-long name fails gracefully.
Thanks,
David
--alex
As far as I see there is no way to read back the name used to
create the transport.
Ok.
Thanks,
David
-----
--alex
Thanks,
David
webrev:
http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open.04/
--alex
On 03/13/2018 16:14, 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.
--alex
