[meta] Assessment of config min/max value range enforcement

!1138 proposed that the min/max value range for a particular option be enforced by the config system's 'range' feature rather than by a block of validation/clipping code within the plugin. After all that's what the feature is meant for, it gives better UX to have ranges specified, and in a lot of cases we already rely upon such enforcement. In response a point was raised that there may be paths in the codebase that "won't enforce min/max/step".

I'm opening this meta issue that I've been putting together since then, primarily just to document the completeness of the current state of enforcement, as I've been going through the code refreshing my knowledge of this. MRs and/or separate specific action issues may be created for anything identified as needing to be done.

Relationship of Config and Variable Systems

Just for the benefit of any reader for whom this is new, or as a refresher, and/or to clarify my use of terminology - a brief introductory overview...

The "config" system consists of a set of configurable options/settings/preferences that are made available to direct user control via CLI and preference interfaces. The overall set consists of one subset defined by the core and further subsets defined by plugins. The config system database captures a model of each option, including name; type; default value; min/max range; choice list (aka value list); labels for interfaces; etc. (See vlc_param and module_config_t). It also holds saved state - the state captured in the saved settings file to be restored upon subsequent program execution. It does not hold temporary runtime state however. Preference interfaces provide a means of editing the saved state.

The "variable" system is a dynamic means of generically capturing and passing around pieces of temporary runtime state to be made available to runtime objects. This can include holding runtime state for config options, but is also sometimes used for various other stuff. Objects that are a part of the VLC object model can hold a set of variable_t objects. These variable_t objects are similar but not identical to config objects. Each object in the VLC object system can be linked to a parent such object forming a tree of related objects. When a value is fetched from a variable object, it is fetched by name from a particular VLC object. Optionally, via what is termed "inheritance", should the object not have one with that name, a search can be done up the tree, ultimately falling back upon the config saved setting value if not found as a variable. A callback system is available for reacting to changes to variable values.

It is a known problem that there is some user confusion over some changes to saved state (via preference interfaces) sometimes taking affect without reloading the app and some not. This relates to how runtime state is cached and propagated throughout the runtime instance in the variable object system, and how in only some cases when retrieving values it will fall back to fetching the saved state value from the config database.

Features Related to Ranges

There are two features related to ranges which deserve a mention.

Step enforcement

The 'step' feature (expressing increments by which a value should increase or decrease by) has nothing directly to do with the config system. It is a feature only of the variable object system. It is thus irrelevant here. In fact there is only one single user of this feature in the entire codebase - the v4l2 access plugin. (We should thus perhaps consider changing that plugin so we can rip this feature out).

Choice/Value lists

'Choice-lists', aka. 'value-lists', express a set of available/valid option values. They provide much better UX in GUI preference interfaces (droplists are used) and in help output, when they are applicable. They are not currently used anywhere in general option value enforcement mechanisms though, unlike ranges.

Consequently it should be understood that generally these lists have no impact upon restricting the value that may be held by an option. While droplist controls in GUI preference interfaces do restrict the values that a user can set there, any value at all (unless restricted by a range - an option can have both) can come from other places like CLI option use, or from a saved settings file. Thus as a general rule it can be sensible to additionally set a range when integer values are contiguous, or otherwise care should be taken to explicitly validate values against the list when using them. It is an easy mistake to make, especially for plugin authors, to assume that a choice list is enforced for them by the core config system, even if it may be reasonable to expect this.

One reason why such lists are not currently enforced may be because not all such lists actually cover every single valid value. Take for instance --freetype-color, which obviously is used to specify a colour. This has a list of a small set of common colours, but valid values are not restricted to this list. This may not actually be the best example, considering the following details, but is the only one that comes to mind. This option has a type of CONFIG_ITEM_RGB which is a sub-type of CONFIG_ITEM_INTEGER and instead of a drop list the Qt interface actually presents a special colour selection control and so actually in this case, changes in the preferences interface are thus not restricted to the list unlike most integer options with lists. As a colour type it also happens to automatically have a range of 0x0 to 0xFFFFFF set. In this case the list only really serves to aid useful help output, though it could be argued that it is unnecessary, that some hints about format and a few examples in the longtext may suffice instead.

It is not clear, without a thorough survey, whether we could drop unnecessary lists like this and then globally enable list enforcement like min/max enforcement, or whether instead we would need to provide a flag to distinguish between options with lists where enforcement would be appropriate and ones where it would not be. (As an aside, for cases where lists are suggestive rather than restrictive, use of a normal droplist control would of course be problematic - is anything actually affected by this?).

Config Range Enforcement Assessment

The central matter of concern here is whether or not ranges are reliably enforced in some way (clipping or rejection) throughout the whole codebase, when values are set in saved config state or runtime variable object state.

Of particular importance is noting the fact that variable objects holding runtime config state are only linked to the config system by way of the variables having the same name as their respective config objects, and there is no helper mechanism by which attributes such as range and value lists get copied into a variable object when creating one to represent a runtime state of a config option. Additionally there's the matter of what gets copied from one variable object to another if/when one is created from another.

It is worth noting that in some cases here and there, value lists are explicitly duplicated into variable objects for the purposes of helping handle cycling of runtime state in response to certain hotkeys.

CLI

Enforced? YES - Ranges are enforced via clipping for option values in CLI use.

For each (known) CLI option used, a variable object is created (on the instance object). The option's min/max range is explicitly copied into it before setting the value via a method that ensures the value gets clipped. Thus ranges are enforced with CLI option values.

Furthermore, with the range copied into the variable object, any hypothetical later changes made via var_*() methods should thus also enforce the range via clipping.

Saved settings file (loading)

Enforced? NO - Ranges are currently not enforced when loading saved settings.

This presents a problem for instance wrt. old values that may previously have been valid, or incorrect values resulting from manual editing.

To be more specific:

• Float options: Ranges are simply not checked at all.
• Integer options: Ranges are actually checked, with a warning printed if the value is out of range, however it is still accepted as it is (no clipping).
• Boolean options: There is a degree of oddness here. These happen to be treated exactly the same as integer options in possibly surprising ways. Upon creation in the config database they are assigned the same default ranges as integer options (INT64_MIN;INT64_MAX); There's nothing to stop a different range being specified in the config definition; and upon reading the value from saved settings, it's just read in exactly as with any other integer, with nothing to ensure that only 0/1 is stored. Not that this particularly matters, but there's no particular reason for this. I haven't noticed any major issues in connection with it but we could handle this better.

GUI advanced preferences (qt, macos)

Enforced? YES - Both by clipping upon saving, and via design of input controls.

Both of these advanced preferences interfaces, when saving options, use the config_PutInt() type functions, which internally perform clipping of the value to the min/max range. Additionally the controls offered to users are designed to take min/max range into account when value-lists are not given, preventing users from cycling controls out of range. (So essentially the clipping upon saving actually achieves nothing, but it goes through it anyway via those function calls).

We can't really remove the clipping from those functions unfortunately because there are some odd uses of these functions in plugins here and there other than the preference interfaces.

GUI simple preferences (qt, macos)

Enforced? YES - Same as advanced view.

The Qt simple view relies upon the same input controls and saving functions (e.g. config_PutInt()) as the advanced view, just with a hard coded set of options. The macos code looks similar. It may perhaps be prudent to go through it with a fine tooth comb just to be certain that there are no option-specific issues, but in general there appears to be no particular problem here.

Effects panel (qt, macos)

Enforced? UNCLEAR - Assessment incomplete at this time.

There's a lot going on in the code of these dialogs, and in particular the Qt code looks like it could do with a good cleanup. There's use of config_PutInt() type functions to do with the 'save' feature which as above involves enforcement. There's also interaction with variable objects, which is a complicated and problematic area in terms of handling enforcement correctly, as discussed later. There are places where min/max and such are being hard coded, which is problematic. In audio code there's duplication of defaults and ranges for various options, with some of those defaults being out of sync with the defaults declared in the option definitions, so that's not great.

It will take additional time to fully assess the code of these dialogs to determine precisely what issues exist here.

Function var_OptionParse()

Enforced? NO - Unimplemented!

While a clipping mechanism is involved, the min/max from the options are not being set in the variable objects, thus the range is not actually enforced.

Function config_ChainParse()

Enforced? YES - Option min/max is set in the variable objects created, and setting the value thus involves clipping.

The function is supplied with a list of option names. For each one it creates a variable object, looks up the config item, copies min/max for int/float types, then sets to work processing options to see if they are in the list, and if so, sets the value in the variable, which will pass through clipping.

Functions var_Set*()

Enforced? YES - Assuming the correct min/max have been copied into the variable object, then yes, clipping is performed upon setting the value.

However, it should be noted that variable objects created via inheriting values from other ones, e.g. via var_CreateGetInteger(), do not inherit min/max or anything else other than the value, so changing the value of such a duplicated object will not be subject to proper range enforcement.

Variable Object Creation Issues

As mentioned in the previous section, the creation of variable objects (variable_t) does not involve copying anything from config objects; this must be done manually when applicable, like for instance how the CLI handling code manually sets min/max, but var_OptionParse() forgets to. Furthermore creation of one object from another does not involve duplication of anything other than the value.

There are many locations within the codebase creating variable objects for holding temporary config option state, and various locations setting the value on those variable objects from untrusted/unreliable sources. It is time consuming and difficult to assess the situation in detail with all of these individual cases (an ongoing effort). However a quick scan of the codebase for ->min.i for instance shows that no setting of min/max seems to be being done anywhere for these instances. This is a big problem.

Defaults and Reset Mechanisms

One further area of concern is the default values for options. It is an easy mistake to make to specify a default config value that is outside of a specified range (and/or not in a value list when it should be). Unfortunately there is nothing in place to catch this upon processing plugin descriptors, or the plugin descriptor cache. So this is another means by which a value could be out of range when it is retrieved via the inheritance based 'get' mechanism.

The config_ResetAll() function used by the 'reset all' preferences option simply copies default values over existing saved values within the config system. No range checking is done (as should be unnecessary anyway). The --reset-config CLI option simply involves re-saving to the saved settings file rather than reading it upon startup.

Summary

While many system components enforce min/max correctly, there are several significant issues that need addressing.

Some proposed actions

1. Fix the incomplete enforcement in var_OptionParse() (simply copy min/max into the variable objects).
2. Add the missing range check for floats in loading the saved settings file.
3. Change the range checks used in loading the saved settings file to clip values. (Inline with other handling; See below for alternative).
4. Add missing check for default value being within specified range when processing plugin descriptors (inc. from plugin cache).
5. Look at having var_Create(), when inheriting, copy min/max into the object. (See below for alternative).

Additional considerations

1. Move from clipping to rejection, or a mix of both?

   • It may not always be appropriate or ideal to silently clip values to a given range rather than rejecting them or at least giving a warning. For instance, rejection would better suit an out of range HTTP port number. On the other hand for some options clipping might be fine if not preferable. I suggest that we consider either switching to a rejection based model, or a mixed model whereby we have a new per-option flag indicating whether or not clipping is allowed.

   • Under the rejection model, CLI processing should either give an error if it encounters an out of range value, or a warning and ignore the value using the default instead. When loading the saved settings, similarly out of range values should be rejected.

   • Under the mixed model, we apply the just described rejection behaviour for strict options, and clipping otherwise.

   • Note that var_SetChecked() already supports the notion of success/failure.

2. Should we add checking of value-lists?

   I strongly feel that this would be a good move, however there's just the question of whether or not we can move to enforcement for all use of lists, with a few like the colour option mentioned above simply being removed as unnecessary, or whether we need a flag to indicate whether or not the list for an option covers all valid values and thus can be enforced.

3. For variable objects that are associated with config options, might it be a good idea to mark them as special in this way, possibly saving a copy of the config item pointer in them, and thus actually reach out to the config item to do validation, avoiding having to wastefully copy min/max and choice lists into the variable object. Note that the action of creating a duplicate variable object (e.g. via var_CreateGetInteger()) would also be efficient.

4. In some cases choice list arrays are duplicated in multiple parts of the codebase, which should be fixed. In particular for cases where this is done for handling cycling of option state in response to hotkey activation, it would be helpful if a 'cycle-value' helper method could simply reach out to the config item to determine the next cyclic value in the list, avoiding such duplication. This obviously relates strongly to the previous point.