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

[Thomas Stüfe]

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> 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
>
>
> 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
> > >>>