RFR(XS): 8212883: Setting a double manageable flag with jcmd/jinfo crashes the JVM

[Tony Printezis]

David,

This is really beyond my sscanf expertise! So I did some quick tests:

“%lf%*s” with “10.0 foo bar” returns 1 (I assume as it filled in only one
item, %*s matched the rest but it didn’t fill in any items)

We can do something like “%lf%c” to detect trailing characters after the
first numeric item. However, this will fail (i.e. return 2) for “10.0 “.

I’m very rusty in using sscanf, so any suggestions on how to improve this
are welcome. Also, yes, let’s do this on another CR. :-)

Tony


—————
Tony Printezis | @TonyPrintezis | tprintezis at twitter.com


On October 24, 2018 at 6:39:18 PM, David Holmes (david.holmes at oracle.com)
wrote:

For the case of <number>+extra stuff e.g "300 400" can't you expand the
matching string to:

"%lf%*s"

so that the additional "junk" counts as a match and so returns > 1 ?

Though this should probably be applied to all cases so perhaps can be a
separate bug fix.

Thanks,
David

On 24/10/2018 11:55 PM, Tony Printezis wrote:
> OK, let me run the jtreg tests on the updated patch just in case and
> I’ll report back later today...
>
>
> —————
> Tony Printezis | @TonyPrintezis | tprintezis at twitter.com
> <mailto:tprintezis at twitter.com>
>
>
> On October 24, 2018 at 9:48:05 AM, Thomas Stüfe (thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>) wrote:
>
>> Hi Tony,
>>
>> I think this can be done as part of the current patch. Thank you!
>>
>> Best Regards, Thomas
>>
>>
>>
>> On Wed, Oct 24, 2018 at 3:37 PM Tony Printezis <tprintezis at twitter.com
>> <mailto:tprintezis at twitter.com>> wrote:
>> >
>> > FWIW, Thomas is correct:
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis 55283
>> >
>> > -XX:CMSAbortablePrecleanWaitMillis=100
>> >
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis=200 55283
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis 55283
>> >
>> > -XX:CMSAbortablePrecleanWaitMillis=200
>> >
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis="" 55283
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis 55283
>> >
>> > -XX:CMSAbortablePrecleanWaitMillis=1
>> >
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis=" " 55283
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis 55283
>> >
>> > -XX:CMSAbortablePrecleanWaitMillis=1
>> >
>> >
>> > Last one “succeeded” because sscanf returned -1 (I confirmed this with
some extra output). Notice however that I could only reproduce it with
jinfo. jcmd VM.set_flag and jconsole both raise appropriate errors.
>> >
>> > One more observation, FYA:
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis 55586
>> >
>> > -XX:CMSAbortablePrecleanWaitMillis=100
>> >
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis="300 400" 55586
>> >
>> > > jinfo -flag CMSAbortablePrecleanWaitMillis 55586
>> >
>> > -XX:CMSAbortablePrecleanWaitMillis=300
>> >
>> >
>> > (sscanf returns 1 as it matches the first integer)
>> >
>> > Again, this only happens with jinfo. jcmd VM.set_flag and console
raise appropriate errors.
>> >
>> > I’m happy to strengthen the condition and compare against 1 (even
though it won’t catch the last issue; I think tools need to do some sanity
checking on the arg before passing it on?). Should I expand this patch? Or
create a new CR?
>> >
>> > Tony
>> >
>> > —————
>> > Tony Printezis | @TonyPrintezis | tprintezis at twitter.com <mailto:
tprintezis at twitter.com>
>> >
>> >
>> > On October 24, 2018 at 4:05:09 AM, Thomas Stüfe (
thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>) wrote:
>> >
>> > On Wed, Oct 24, 2018 at 8:57 AM David Holmes <david.holmes at oracle.com
<mailto:david.holmes at oracle.com>> wrote:
>> > >
>> > > Hi Thomas,
>> > >
>> > > On 24/10/2018 4:32 PM, Thomas Stüfe wrote:
>> > > > Hi,
>> > > >
>> > > > I am really not sure about the implicit boolean comparison of the
>> > > > sscanf return value (not only for this but the other flag variants
>> > > > too).
>> > >
>> > > It's checking for 0 or 1 matches. But yes hotspot style says it
should
>> > > be an explicit check against 0 or 1.
>> > >
>> > > > sscanf may return 0-n (for 0-n matched items) and EOF in case of
an
>> > > > error. Is EOF always 0? Otherwise, to be sure, I would compare the
>> > > > return value with 1 since we expect 1 item to match.
>> > >
>> > > That's probably technically safer, though for sscanf I don't see how
you
>> > > can return EOF unless the arg is an empty string - even then that
may
>> > > simply constitute 0 matches.
>> >
>> > Posix: " If the input ends before the first matching failure or
>> > conversion, EOF shall be returned. "
>> >
>> > Seems, with glibc at least, EOF is -1, and it is returned if you never
>> > get the chance to at least match one item because of missing input.
>> >
>> > 3 int main(int argc, char* argv) {
>> > 4 void* p;
>> > 5 int result = sscanf("", "%p", &p);
>> > 6 printf("%d ", result);
>> > 7 if (result == EOF) {
>> > 8 printf("EOF");
>> > 9 }
>> > 10 return 0;
>> >
>> > return -1 EOF
>> >
>> > Cheers, Thomas
>> >
>> >
>> > >
>> > > Cheers,
>> > > David
>> > >
>> > > > Just my 5c.
>> > > >
>> > > > Cheers, Thomas
>> > > >
>> > > >
>> > > >
>> > > > On Wed, Oct 24, 2018 at 1:16 AM David Holmes <
david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>> > > >>
>> > > >> Hi Tony,
>> > > >>
>> > > >> On 24/10/2018 8:13 AM, Tony Printezis wrote:
>> > > >>> Webrev here:
>> > > >>>
>> > > >>> http://cr.openjdk.java.net/~tonyp/8212883/webrev.0/
>> <http://cr.openjdk.java.net/%7Etonyp/8212883/webrev.0/>
>> > > >>>
>> > > >>> Currently, HotSpot doesn’t actually have any double manageable
flags, which
>> > > >>> is why I think no-one has hit this before. I recently added a
couple to our
>> > > >>> own builds and I noticed that setting them is not handled
correctly in the VM.
>> > > >>> The fix is pretty trivial (mostly cut-and-paste from what the
code does for
>> > > >>> the other types).
>> > > >>
>> > > >> I agree the fix is pretty obvious and straight-forward.
>> > > >>
>> > > >>> I tested it by introducing a dummy double manageable flag and I
can set it
>> > > >>> with jinfo/jcmd and jconsole (these cover all the various paths
in the
>> > > >>> changes). Is it worth expanding the
>> > > >>> serviceability/attach/AttachSetGetFlag.java test to also get/set
a double
>> > > >>> flag (I’d need to introduce a dummy double manageable flag to do
that
>> > > >>> though)?
>> > > >>
>> > > >> I hate to see new code untested ... but then it seems we don't
have
>> > > >> tests for all the existing types of flags anyway.
>> > > >>
>> > > >> Reviewed.
>> > > >>
>> > > >> Thanks,
>> > > >> David
>> > > >>
>> > > >>> Regards,
>> > > >>>
>> > > >>> Tony
>> > > >>>
>> > > >>>
>> > > >>> —————
>> > > >>> Tony Printezis | @TonyPrintezis | tprintezis at twitter.com <mailto:
tprintezis at twitter.com>
>> > > >>>