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

[Kim Barrett]

On May 27, 2015, at 5:28 PM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
> 
> Here is a revision 2 of the feature taking into account feedback from Dmitry, David, Kim and Alexander. 
> 
> […]
> References: 
> 
>           Webrev: http://cr.openjdk.java.net/~gziemski/8059557_rev2 
>             note: due to "awk" limit of 50 pats the Frames diff is not available for "src/share/vm/runtime/arguments.cpp” 

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.

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

------------------------------------------------------------------------------  
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);
  }

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

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.

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

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

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

What's this about?

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