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

Tony Printezis tprintezis at twitter.com
Thu Oct 25 22:46:23 UTC 2018


Guilty as charged. :-)

I’ll also do a submit repo submission, if anything else to remind myself
how to do that…

Tony


—————
Tony Printezis | @TonyPrintezis | tprintezis at twitter.com


On October 25, 2018 at 1:34:39 PM, coleen.phillimore at oracle.com (
coleen.phillimore at oracle.com) wrote:



On 10/25/18 11:08 AM, Gerard Ziemski wrote:
> hi Tony,
>
> Thank you for fixing the sscanf() in
src/hotspot/share/services/writeableFlags.cpp
>
> I’m running your patch with hs_tier1,2,3,4,5,6 tests just to make sure
all is good.
>
> Do you have a JDK committer on your team, or are you one, or do you need
a sponsor?

Tony is a Reviewer so let him know when the testing is done, and he can
push this directly.
thanks,
Coleen

>
>
> cheers
>
>> On Oct 25, 2018, at 8:03 AM, 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
>>>>>>>>>


More information about the hotspot-runtime-dev mailing list