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

Gerard Ziemski gerard.ziemski at oracle.com
Thu Oct 25 15:08:58 UTC 2018 

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?


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