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

Gerard Ziemski gerard.ziemski at oracle.com
Thu Jun 4 15:20:57 UTC 2015

hi Derek,

I answered your questions below:

On 6/3/2015 4:29 PM, Derek White wrote:

Hi Gerard,

Thanks for the explanations. Just quoting still-open issues below: On 6/2/15 4:09 PM, Gerard Ziemski wrote: Thank you Derek very much for your feedback. Please see my answers inline:

Line 2036, 2041: - Set status to false instead of returning directly? I’m sorry, which file do those lines refer to? In arguments.cpp, in bool Arguments::checkvmargsconsistency(). My point is that the pre-existing control flow was to set "status = false" and continue checking flags, not to bail out when the first failure occurred.

Excellent catch, fixed.

Line 2827 (existing); Change "K" -> "1*K" for consistency. Line 2827 in gobals.hpp or is it a different file? Never mind. I should have been talking about arguments.cpp, but I must have miss-typed the line #, and now I can't find it. There are actually lots of naked "K"s around, so even though I'm not a fan, I can't complain about consistency. - Shouldn't the default implementations for CommandLineFlagConstraint::applyXXX() return failure, not success? I looked at it from the point of view of the more likely scenario, which is assume success, unless a fault detected.

If we did do this, though, then instead of: Flag::Error MinHeapFreeRatioConstraintFunc(bool verbose, uintx* value) { if ((CommandLineFlags::finishedInitializing()) && (*value > MaxHeapFreeRatio)) { if (verbose == true) { jiofprintf(defaultStream::errorstream(), "MinHeapFreeRatio (" UINTXFORMAT ") must be less than or " "equal to MaxHeapFreeRatio (" UINTXFORMAT ")\n", *value, MaxHeapFreeRatio); } return Flag::VIOLATESCONSTRAINT; } else { return Flag::SUCCESS; } } we’d have: Flag::Error MinHeapFreeRatioConstraintFunc(bool verbose, uintx* value) { Flag::Error error = Flag::VIOLATESCONSTRAINT; if ((CommandLineFlags::finishedInitializing()) && (*value > MaxHeapFreeRatio)) { if (verbose == true) { jiofprintf(defaultStream::errorstream(), "MinHeapFreeRatio (" UINTXFORMAT ") must be less than or " "equal to MaxHeapFreeRatio (" UINTXFORMAT ")\n", *value, MaxHeapFreeRatio); } } else { error = Flag::SUCCESS; } return error; } which is not as nice, in my opinion. I'm not sure we're talking about the same thing. Looking at commandLineFlagConstraintList.hpp: 55 virtual Flag::Error applybool(bool* value, bool verbose = true) { return Flag::SUCCESS; }; I had the question "What happens if someone calls a version of the apply function (applyFOO) that doesn't match the type of the constraint?" For example, look at CommandLineFlagConstraint::applybool(). What if the flag is really an "int", and it has a constraint? So the constraint is of type CommandLineFlagConstraintintx. If applybool() is ever called on a CommandLineFlagConstraintintx it will return Flag::SUCCESS. This is wrong - it violates the implicit constraint that the type of the value matches the type of the flag. It looks like the callers ensure that constraint's type matches the value's type, so the default applybool() is never called. I'm not sure why you'd have to re-write MinHeapFreeRatioConstraint() (as describe above) no matter what the default value of applybool() is. But if some later code forgets to match the types correctly, I think we're better off calling shouldNotReachHere() in the default methods. I think Kim had a similar question about default checkFOO() methods in commandLineFlagRangeList.hpp. 47 virtual Flag::Error checkintx(intx value, bool verbose = true) { return Flag::SUCCESS; } 48 virtual Flag::Error checkuintx(uintx value, bool verbose = true) { return Flag::SUCCESS; } 49 virtual Flag::Error checkuint64t(uint64t value, bool verbose = true) { return Flag::SUCCESS; } 50 virtual Flag::Error checksizet(sizet value, bool verbose = true) { return Flag::SUCCESS; } 51 virtual Flag::Error checkdouble(double value, bool verbose = true) { return Flag::SUCCESS; } Thanks for reading this far! - Derek Notes: applybool() is called in two places. 1) applyconstraintandcheckrangebool() - Do the callers do a type check? - Yes. On type mismatch: - CommandLineFlags::boolAtPut(const char* name, sizet len, bool* value, Flag::Flags origin) returns Flag::WRONGFORMAT. - CommandLineFlagsEx::boolAtPut(CommandLineFlagWithType flag, bool value, Flag::Flags origin) will fail a "guarantee". Not sure if that's best idea. 2) CommandLineFlags::checkallrangesandconstraints() - Caller basically does a type case, so applybool() is never called with wrong type flag. 55 virtual Flag::Error applybool(bool* value, bool verbose = true) { return Flag::SUCCESS; }; 56 virtual Flag::Error applyintx(intx* value, bool verbose = true) { return Flag::SUCCESS; }; 57 virtual Flag::Error applyuintx(uintx* value, bool verbose = true) { return Flag::SUCCESS; }; 58 virtual Flag::Error applyuint64t(uint64t* value, bool verbose = true) { return Flag::SUCCESS; }; 59 virtual Flag::Error applysizet(sizet* value, bool verbose = true) { return Flag::SUCCESS; }; 60 virtual Flag::Error applydouble(double* value, bool verbose = true) { return Flag::SUCCESS; These are never called (because the callers ensure that constraint's type matches the "value" type), so it shouldn't matter if they return Flag::SUCCESS or Flag::INVALIDFLAG.

Ah, now I see what we're talking about - thank you for clarifying.

Fixed with ShouldNotReachHere()

I will be posting a new webrev shortly, after I do testing.

cheers

More information about the hotspot-dev mailing list