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

[David Holmes]

On 25/10/2018 10:54 PM, Tony Printezis wrote:
> 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)

I didn't read the docs closely enough:

    An optional '*' assignment-suppression character: scanf() reads 
input as directed by the conversion specification, but discards the 
input.  No corresponding pointer argument is required, and  this 
specification is _not included in the count of successful assignments 
returned by scanf()._

That's a shame. Seemed such an obvious way to ensure no trailing junk.

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

Right - it becomes painful.

> 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. :-)

I won't push any more :)

Thanks,
David

> Tony
> 
> 
> —————
> Tony Printezis | @TonyPrintezis | tprintezis at twitter.com 
> <mailto:tprintezis at twitter.com>
> 
> 
> On October 24, 2018 at 6:39:18 PM, David Holmes (david.holmes at oracle.com 
> <mailto: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>
>> > <mailto: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>
>> > <mailto: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>
>> >> <mailto: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> 
>> <mailto: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> 
>> <mailto: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> 
>> <mailto: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> 
>> <mailto: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/>
>> >> <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> 
>> <mailto:tprintezis at twitter.com <mailto:tprintezis at twitter.com>>
>> >> > > >>>