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

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