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

[Tony Printezis]

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