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

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