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

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