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

[David Holmes]

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