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

Kim Barrett kim.barrett at oracle.com
Wed Jun 3 22:08:14 UTC 2015

On Jun 3, 2015, at 3:26 PM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:

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

34 #define TEMPBUFSIZE 256 ... 61 static void printflagerrormessageifneeded(Flag::Error error, const char* name, FormatBuffer<80>& errmsg) { The ultimate destination of the error message is errmsg, which is explicitly limited to 80 characters. It seems kind of pointless to make TEMPBUFSIZE larger than the errmsg 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 checkintx(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 FLAGSET{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 FLAGSETxxx 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 FLAGSETCMDLINE 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 FLAGSETDEFAULT and FLAGSETERGO 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...

More information about the hotspot-dev mailing list