Request for code review (XS)

[Volker Simonis]

Hi Tom,

sorry for the delay, I somhow missed your post.

I don't think that the NULL check you suggested is enough to fix the
problem because the actual crash happens in
'CommandLineFlags::ccstrAtPut()' where 'strlen()' is called with a
NULL argument because 'value' is set to NULL in 'Arguments::parse_argument()'.

I even think that the NULL check in "arguments.cpp" is unnecessary,
because at that point 'value' can't be NULL (as far as I see). Then
better put the NULL check into 'CommandLineFlags::ccstrAtPut()'. This
is exaclty what my new patch does (besides fixing the default ccstr
arguments in globals.hpp to conform to the rules detailed by you.)

Any comments?
Volker

On 9/5/08, Tom Rodriguez <Thomas.Rodriguez at sun.com> wrote:
> I'm not sure that fix is right.  I struggled a bit with how to do this
> consistently when I made the change which caused the breakage.  There are at
> least two kinds of string arguments that we support, single argument and
> accumulating.  LogFile is an example of a single argument one where
> specifying it multiple times results in the last value being chosen.  These
> are declared using ccstr.  Accumulating ones are like CompileOnly, where
> specifying it multiple times results in the string being concatenated.
> These are declared as ccstrlist.  Single argument ones normally should have
> their default value be NULL instead of "" and accumulating ones use "".  So
> the intent of that code is that -XX:Foo="" should be equivalent setting foo
> to NULL.  I think there should be a check null in the existing code, like
> this:
>
>  diff --git a/src/share/vm/runtime/arguments.cpp
> b/src/share/vm/runtime/arguments.cpp
>  --- a/src/share/vm/runtime/arguments.cpp
>  +++ b/src/share/vm/runtime/arguments.cpp
>  @@ -610,7 +610,7 @@ bool Arguments::parse_argument(const
> cha
>        if (flag->ccstr_accumulates()) {
>          return append_to_string_flag(name, value, origin);
>        } else {
>  -        if (value[0] == '\0') {
>  +        if (value != NULL && value[0] == '\0') {
>            value = NULL;
>          }
>          return set_string_flag(name, value, origin);
>
>  I suspect these default initializer rules should be enforced by the globals
> code.
>
>  tom
>
>
>  On Sep 5, 2008, at 5:52 AM, Keith McGuigan wrote:
>
>
> >
> > Hello,
> >
> > I need a couple of reviews of this patch submitted by Volker Simonis
> (attached).  It a small fix to the argument processing.   I would review it
> myself, but jcheck won't let me both be the submitter and the reviewer, so I
> need a couple of reviews.
> >
> > Thanks.
> >
> > --
> > - Keith
> >
> > From: Volker Simonis <volker.simonis at gmail.com>
> > Date: September 4, 2008 7:03:22 AM PDT
> > To: hotspot-dev at openjdk.java.net
> > Subject: HotSpot segfaults if given -XX options with an empty string
> argument (regression)
> >
> >
> > Hi,
> >
> > I just realized that there's a regression in the argument parsing code
> > of the HotSpot which leads to a segmentation fault if an -XX option
> > with an empty string argument (e.g. -XX:SyncKnobs= or
> > -XX:SyncKnobs="") is given on the command line.
> >
> > The regression must have appeared somewhere in HS 11, because Java
> > 1.6.0_06 (which contains HS 10.0_b22) doesn't show the problem while
> > both Java 1.6.0_10 (HS 11.0_b11) and Java 1.7.0-ea-b24 (HS 12.0_b01)
> > as well as the latest OpenJDK snapshot (Rev. 292 with tag jdk7-b34)
> > are affected.
> >
> > Attached you can find a patch against
> > http://hg.openjdk.java.net/jdk7/jdk7/hotspot
> >
> > At the same time, the patch also fixes a minor flaw in globals.hpp
> > where the default value of the 'PrintAssemblyOptions' option, which is
> > of type 'ccstr', should really be "" instead of 'false'.
> >
> > Regards,
> > Volker
> > <hg.patch>
> >
> >
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hotspot_294_1ed580ebcdba.patch
Type: text/x-patch
Size: 3945 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20080909/41fb0af7/attachment.bin