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

[Kim Barrett]

On Jun 3, 2015, at 3:26 PM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
> 
>> ------------------------------------------------------------------------------ 
>> 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.
>> 
> I need large enough buffer to hold the raw range string, which I then “compress” by removing white spaces, so that it fits into the 80 character error buffer.

I don't see that.  What I see is output of the range into a local
stringStream, with whitespace elimination occurring in the transfer of
data from that stringStream to the buffer.  The undesired whitespace
never makes it into the buffer.

>> ------------------------------------------------------------------------------ 
>> 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. 
>> 
> The name arguments for the constructors come from macro tables and are string literals.

Yes, but there's no assurance of that, or even any indication that it
must be so.  At least some comments would make me feel a little
better.

>> ------------------------------------------------------------------------------   
>> 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. 
>> 
> I’m just mimicking the pre-existing behavior, which was with no checking everything was passing. The way I see it, we pass unless we prove we violate constraint or range.

See Derek’s followup and my reply to that.

>> 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.
>> 
> Actually, most FLAG_SET_CMDLINE uses check the return values, except for a few cases where a flag is deprecated, or some used in some unit tests, that I left up to the team responsible.

I think there are 10 of these.  I haven’t audited them any further than discovery.

cd hotspot/src
egrep -R " FLAG_SET_CMDLINE\(" * | grep -v "#define FLAG_SET_CMDLINE”
flags are: MaxRAMFraction, CreateCoredumpOnCrash, NewSize, OldSize, MaxNewSize

> I assumed that the uses of FLAG_SET_DEFAULT and FLAG_SET_ERGO know what that are doing, but if it’s a wrong assumption, then we can add error check there too later.
> 
> This is an initial effort of introducing the new mechanism in. There are already subtasks covering pending tasks such as implementing those flags that don’t have ranges yet, so this checkin doesn’t represent the final effort.

I don't think that's a safe assumption; bugs happen.  But I'm fine
with addressing that being a followup task.  Just make sure that task
exists...