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

Kim Barrett kim.barrett at oracle.com
Tue Jun 2 22:52:40 UTC 2015

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/8059557rev2 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_ 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, 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::AtPut(const char* name, ...) functions, the call to the associated apply_constraint_and_check_range_ 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.

More information about the hotspot-dev mailing list