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:
> 
> hi all, 
> 
> 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” 

Here's another collection of comments.  I haven't looked at the actual
ranges and constraints yet.  I might leave off that, since others seem
to be looking there.  Let me know if you think another set of eyes is
needed there.

------------------------------------------------------------------------------ 
src/share/vm/runtime/globals.cpp  
1067 //#define PRINT_FLAGS_SORTED_BY_TYPE_THEN_NAMES
and following

The PRINT_FLAGS_SORTED_BYTE_TYPE_THEN_NAMES is never defined, so the
associated #else clause is (presently) dead code.  If it were live
code then I'd have several complaints about it.  But since it appears
to be dead, I'll limit my complaint to it existing at all.

------------------------------------------------------------------------------
src/share/vm/runtime/commandLineFlagRangeList.cpp

There is a large amount of code near-duplication among the various
CommandLineFlagRange_<type> classes.  I think this could be
substantially reduced via some fairly straightforward usage of macros
and templates.  This can be dealt with in a follow-on cleanup - please
file a CR.

Probably similarly for the constraint classes.

------------------------------------------------------------------------------
src/share/vm/runtime/commandLineFlagConstraintList.cpp  
src/share/vm/runtime/commandLineFlagRangeList.cpp 

The macros EMIT_CONSTRAINT_COMMERCIAL_FLAG and
EMIT_RANGE_COMMERCIAL_FLAG are defined in these files.  This is
parhaps the wrong place for them, since they are unused in open code.

If moving the commercial flag handlers, it might be desirable to
define a helper macro for use by all the EMIT_RANGE_xxx_FLAG (and
similarly for EMIT_COMMERCIAL_xxx_FLAG).

------------------------------------------------------------------------------ 
src/share/vm/runtime/commandLineFlagConstraintList.cpp 
src/share/vm/runtime/commandLineFlagRangeList.cpp 

The classes CommandLineFlag{Constraint,Range}List are derived from
CHeapObj<mtInternal>, but appear to never be allocated, and only have
static members.  I think they should instead be derived from
AllStatic.

------------------------------------------------------------------------------
src/share/vm/runtime/commandLineFlagRangeList.cpp 

The various emit_range_xxx functions are defined with global scope.
I *think* they should be defined at file scope.

Similarly for the constraint list functions.

------------------------------------------------------------------------------
src/share/vm/runtime/commandLineFlagRangeList.cpp 
src/share/vm/runtime/commandLineFlagRangeList.cpp 

These files directly include runtime/os_ext.hpp, but that file is not
intended to be directly included, but must be included from the
*middle* of runtime/os.hpp.  I think the only reason this include
isn't blowing up is that there is some earlier include of
runtime/os.hpp, so that the direct include of os_ext.hpp is suppressed
by its include guard.  It's not obvious what earlier file might be
providing that include of os.hpp, and I wonder if perhaps it's coming
from the precompiled headers.  Which reminds me, has this changeset
been tested without precompiled headers?

That include of os_ext.hpp is to obtain the definitions of the macros
EMIT_{CONSTRAINTS,RANGES}_FOR_GLOBALS_EXT.  I think those macro
definitions probably ought to be moved to a different place, though I
don't have a suggested landing place.

------------------------------------------------------------------------------ 
src/share/vm/services/writeableFlags.cpp

34 #define TEMP_BUF_SIZE 256
...
61 static void print_flag_error_message_if_needed(Flag::Error error, const char* name, FormatBuffer<80>& err_msg) {

The ultimate destination of the error message is err_msg, which is
explicitly limited to 80 characters.  It seems kind of pointless to
make TEMP_BUF_SIZE larger than the err_msg size.

------------------------------------------------------------------------------  
src/share/vm/runtime/globals.cpp

In the various CommandLineFlags::<type>AtPut(const char* name, ...)
functions, the call to the associated
apply_constraint_and_check_range_<type> helper function uses
  (CommandLineFlags::finishedInitializing() == false)
to compute the verbose argument in that helper function call.  Better
would be simply
  !CommandLineFlags::finishedInitializing()

------------------------------------------------------------------------------ 
src/share/vm/runtime/commandLineFlagRangeList.hpp 
42 public:
43  const char* _name;

_name should be private.  Any external accesses should be via the
existing public name() member function.

Similarly for CommandLineFlagConstraint. 

------------------------------------------------------------------------------ 
src/share/vm/runtime/commandLineFlagRangeList.hpp 
44  CommandLineFlagRange(const char* name) { _name=name; }

This will result in a dangling pointer if the name argument can be
deallocated.  I suspect the intent is that this is only called with a
string literal, and that all callers do so, but that's hard to ensure.

Similarly for CommandLineFlagConstraint. 

I'm not sure there's anything to do here, other than audit and add
comments. 

------------------------------------------------------------------------------   
src/share/vm/runtime/commandLineFlagRangeList.hpp  
47   virtual Flag::Error check_intx(intx value, bool verbose = true) { return Flag::SUCCESS; }
... and other similar functions ...
... and similar functions in CommandLineFlagConstraint ...

I'm dubious about default implementations returning success.  I think
the default should be a failure indication, possibly even with
ShouldNotReachHere(), since it indicates an attempt to apply a
constraint for the wrong type of value.  If the constraint were being
applied to the correct type of value, the default implementation would
be overridden by the derived class of the constraint object.

Similarly for CommandLineFlagConstraint. 

------------------------------------------------------------------------------  

If I'm understanding correctly, there should now never be a call to
FLAG_SET_{CMDLINE,DEFAULT,ERGO,other?} that doesn't have its result
checked.  But most (by a substantial majority) appear to be unchecked.
I'm guessing that's a followup task?  I don't see any mention of
FLAG_SET_xxx checking in the review summary; it's a different task
from determining appropriate ranges for flags that haven't had that
done yet.

------------------------------------------------------------------------------   
src/share/vm/runtime/globals.cpp
331  if (printRanges == false) {

I'd prefer "if (!printRanges) {".

Similarly for

380  } else if ((is_bool() == false) && (is_ccstr() == false)) {
381
381    if (printRanges == true) {

Searching for " == true" and " == false" will probably find more.

------------------------------------------------------------------------------