Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
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 print_on(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, size_t 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