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

Kim Barrett kim.barrett at
Thu May 28 22:10:15 UTC 2015

On May 27, 2015, at 5:28 PM, Gerard Ziemski <gerard.ziemski at> wrote:
> Here is a revision 2 of the feature taking into account feedback from Dmitry, David, Kim and Alexander. 
> […]
> References: 
>           Webrev: 
>             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.

 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

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.

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

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

 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

Similarly for line 195. 

Similarly for src/share/vm/services/memoryService.cpp:521

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

  88     buffer[79] = '\0';

What's this about?

 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.

More information about the hotspot-dev mailing list