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

David Holmes david.holmes at oracle.com
Thu Oct 25 23:50:45 UTC 2018


That looks fine to me.

Thanks,
David

On 25/10/2018 11:03 PM, Tony Printezis 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
>>>>>>>


More information about the hotspot-runtime-dev mailing list