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

Tony Printezis tprintezis at twitter.com
Wed Oct 24 13:55:38 UTC 2018

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