Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments (original) (raw)

Gerard Ziemski gerard.ziemski at oracle.com
Mon Jun 1 21:32:33 UTC 2015

Thank you Kim for a review. I provided answers to the raised issues in-line:

Subject: Re: Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments Date: Thu, 28 May 2015 18:10:15 -0400 From: Kim Barrett<kim.barrett at oracle.com> To: Gerard Ziemski<gerard.ziemski at oracle.com> CC: hotspot-dev at openjdk.java.net Developers<hotspot-dev at openjdk.java.net>, David Holmes<david.holmes at oracle.com>, Coleen Phillimore<coleen.phillimore at oracle.com>, Dmitry Dmitriev<dmitry.dmitriev at oracle.com>, Alexander Harlap<alexander.harlap at oracle.com>

This is only a partial review. I haven't taken a detailed look at arguments.[ch]pp or globals.cpp, and haven't looked at the new range and constraint checking or the tests. But I'm running out of steam for today, and won't be able to get back to this until Monday, so giving you what I have so far. ------------------------------------------------------------------------------ Many files are being changed to #include globals.hpp just to get access to Flag values. Perhaps globals.hpp ought to be split up. This probably ought to be addressed as a separate CR though.

Filed ashttps://bugs.openjdk.java.net/browse/JDK-8081519

------------------------------------------------------------------------------ src/share/vm/runtime/globals.hpp 362 void printon(outputStream* st, bool printRanges = false, bool withComments = false ); 458 static void printFlags(outputStream* out, bool withComments, bool printRanges = false);

Why are the withComments and printRanges arguments in different orders? Just in general, multiple bool arguments tend to be problematic for understanding the using code. Common alternative is to use enums. Another common alternative (used often in hotspot) is to use comments in the calls; that helps the reader, so long as the (not checked by compiler) order is actually as commented. Calls should at least be commented, for consistency with other hotspot usage. I had originally implemented the API with “printRanges” argument before “withComments”, however, there are quite few places using this API, so I decided to go with simpler changes, by ordering the arguments the way I did and assigning the default value of false, which allowed me not to touch all the places this API was used.

“print_on” is an SPI and I did not change it to reflect the “printFlags” order, because that’s how I originally had it in “printFlags”, and then forgot to keep them in sync.

I added comments and changed the order of arguments of “print_on” to match that of “printFlags” for consistency.

------------------------------------------------------------------------------ src/share/vm/runtime/init.cpp 145 if (PrintFlagsFinal) { 146 CommandLineFlags::printFlags(tty, false); 147 } 148 if (PrintFlagsRanges) { 149 CommandLineFlags::printFlags(tty, false, true); 150 } [145-147 from original, 148-150 added]

Really print the flags twice if both options are provided? I was expecting something like: if (PrintFlagsFinal || PrintFlagsRanges) { CommandLineFlags::printFlags(tty, false, PrintFlagsRanges); } Done.

------------------------------------------------------------------------------ src/share/vm/runtime/os.hpp 167 // A strlcat like API for safe string concatenation of 2 NULL limited C strings 168 // strlcat is not guranteed to exist on all platforms, so we implement our own 169 static void strlcat(char *dst, const char *src, sizet size) { 170 register char *dst = dst; 171 register char *src = (char *)src; 172 register int size = (int)size; 173 174 while ((size-- != 0) && (*dst != '\0')) { 175 dst++; 176 } 177 while ((size-- != 0) && (*src != '\0')) { 178 *dst = *src; 179 dst++; src++; 180 } 181 *dst = '\0'; 182 }

Several problems here: 1. In the description comment: NULL is a pointer. The string terminator is NUL. 2. Use of "register" storage class specifiers: The "register" storage class is deprecated in C++11 and slated for removal in C++17. Compilers have for a long time been parsing it but otherwise ascribing no special meaning beyond specifying automatic storage allocation. 3. There's no reason for any of the casts. 4. Use of prefixed names is generally reserved for member variables in hotspot. There's no need for these additional variables anyway, after elimination of the register storage class usage and the inappropriate casts. 5. This will buffer overrun if strlen(dst) >= size. This differs from BSD strlcat. Quite frankly I feel pretty embarrassed that there was a bug in the previous implementation. I have taken the various feedback into account and came up with a better one. I have asked for help making sure the new implementation has no bugs.

6. Unlike BSD strlcat, this doesn't provide a return value that allows the caller to detect truncation. I would think most real uses aught to care about that case, though I haven't audited uses in this change set yet.

The new code doesn’t care about the truncation.

------------------------------------------------------------------------------ src/share/vm/services/classLoadingService.cpp 184 bool succeed = (CommandLineFlags::boolAtPut((char*)"TraceClassLoading", &verbose, Flag::MANAGEMENT) == Flag::SUCCESS); 185 assert(succeed, "Setting TraceClassLoading flag fails");

I'm surprised this doesn't produce an unused variable warning in a product build. Rather than converting the boolAtPut result to a bool success and asserting it to be true, it would be better to capture the resulting Flag::Error, test for success in the assert, and report the failure reason in the assert message. Pre-existing defect: Unnecessary cast. Possible pre-existing defect: I don't understand exactly how these service functions are used, but I wonder whether an assertion is really the appropriate method to check for failure to perform the operation. Similarly for line 195. Similarly for src/share/vm/services/memoryService.cpp:521 I fixed those the best I could, but I left the asserts in place to keep the same behavior.

------------------------------------------------------------------------------ src/share/vm/services/writeableFlags.cpp 62 if (error != Flag::SUCCESS) { ... 93 }

Rather than if != success and indenting the whole function, I think it would be more readable if this were if (error == Flag::SUCCESS) { return; } ... Done.

------------------------------------------------------------------------------ src/share/vm/services/writeableFlags.cpp 88 buffer[79] = '\0';

What's this about?

Debugging leftover - removed.

------------------------------------------------------------------------------ src/share/vm/services/writeableFlags.cpp 108 Flag::Error err = CommandLineFlags::boolAtPut((char*)name, &value, origin); 125 Flag::Error err = CommandLineFlags::intxAtPut((char*)name, &value, origin); 142 Flag::Error err = CommandLineFlags::uintxAtPut((char*)name, &value, origin); ... and so on ...

Pre-existing inappropriate casts. Cleaned.

Thank you Kim for the detailed feedback - I will be posting up the new webrev soon, so please take a look.

cheers

More information about the hotspot-dev mailing list