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

[Tony Printezis]

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


On October 24, 2018 at 4:05:09 AM, Thomas Stüfe (thomas.stuefe at gmail.com)
wrote:

On Wed, Oct 24, 2018 at 8:57 AM David Holmes <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>
wrote:
> >>
> >> Hi Tony,
> >>
> >> On 24/10/2018 8:13 AM, Tony Printezis wrote:
> >>> Webrev here:
> >>>
> >>> http://cr.openjdk.java.net/~tonyp/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
> >>>