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

[Thomas Stüfe]

This looks fine to me. Thank you, Tony.

..Thomas
On Thu, Oct 25, 2018 at 3:04 PM Tony Printezis <tprintezis at twitter.com> wrote:
>
> New webrev:
>
> http://cr.openjdk.java.net/~tonyp/8212883/webrev.1/
>
> Only difference vs the previous one is the
>
> if (sscanf(…) == 1) { ...
>
> checks. I got no failures running the hotspot and jdk jtreg tests.
>
> Tony
>
>
> —————
> Tony Printezis | @TonyPrintezis | tprintezis at twitter.com
>
>
> On October 24, 2018 at 1:07:45 PM, Gerard Ziemski (gerard.ziemski at oracle.com) wrote:
>
> Thank you Tony for doing this.
>
> BTW I checked the usage of sscans in hotspot and in all instances, except src/hotspot/share/services/writeableFlags.cpp, we seem to check for the return value >0 and we treat anything other value as an error.
>
>
> cheers
>
> > On Oct 24, 2018, at 8:55 AM, Tony Printezis <tprintezis at twitter.com> 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
> >
> >
> > On October 24, 2018 at 9:48:05 AM, Thomas Stüfe (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>
> > 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
> >>>>>>
>