gopts: validation of CLI options not working

[qwepoizt]

This issue is currently WIP.

When using the HL API and the gopts plugin, CLI options passed to the application are not validated.

3 classes of validation errors

The following three are not detected:

1. Class 1: missing argument (forked to gopts: No error on missing argument  #4042)
2. Class 2: wrong type (forked to gopts: Wrong type detected too late #4043)
3. Class 3: validation errors reported by check/* plugins (forked to gopts: check/ plugins perform no validation during kdbGet() #4049)

Depending on the causes - which are yet to be identified - this issue should probably be split up into individual issues.

Steps to Reproduce the Problem

0. Execute the steps below which follow the tutorial High-level API (with code-generation).

1. Create file test.ini with content:

[]
mountpoint = test.ecf

[myboolean]
type = boolean
default = 1
example = 0
opt/long = mb
opt/arg = required

[myenum]
type = enum
check/enum = #1
check/enum/#0 = ok1
check/enum/#1 = ok2
default = ok1
example = ok2
opt/long = me
opt/arg = required

[myfloatcheckrange]
type = float
check/type = float
check/range = 0.0-1.0
default = 1.0
example = 1.0
opt/long = mfcr
opt/arg = required

[mystringcheckdate]
type = string
check/date = ISO8601
check/date/format = timeofday
default = 00:00
example = 01:00
opt/long = mscd
opt/arg = required

[mystringcheckvalidation]
type = string
check/validation = ^[a]{3}$
default = aaa
example = aaa
opt/long = mscv
opt/arg = required

[myunsignedshortcheckrange]
type = unsigned_short
check/type = unsigned_short
check/range = 0-20
default = 10
example = 10
opt/long = muscr
opt/arg = required

2. Execute kdb gen -F ni=test.ini highlevel "/sw/example/myapp/#0/current" conf

3. Create file myapp.c with content:

#include <stdio.h>
#include <elektra.h>
#include "conf.h"

int main(int argc, const char * const * argv, const char * const * envp) {
    exitForSpecload (argc, argv);
    ElektraError * error = NULL;
    Elektra * elektra = NULL;
    int rc = loadConfiguration (&elektra, argc, argv, envp, &error);

    if (rc == -1)
    {
        fprintf (stderr, "An error occurred while opening Elektra: %s", elektraErrorDescription (error));
        elektraErrorReset (&error);
        exit (EXIT_FAILURE);
    }

    if (rc == 1)
    {
        // help mode - application was called with '-h' or '--help'
        // for more information see "Command line options" below
        printHelpMessage (elektra, NULL, NULL);
        elektraClose (elektra);
        exit (EXIT_SUCCESS);
    }

    fprintf(stdout, "myboolean is: %d \n", elektraGetMyboolean(elektra));
    fprintf(stdout, "myenum is: %d \n", elektraGetMyenum(elektra));
    fprintf(stdout, "myfloatcheckrange is: %f \n", elektraGetMyfloatcheckrange(elektra));
    fprintf(stdout, "mystringcheckdate is: %s \n", elektraGetMystringcheckdate(elektra));
    fprintf(stdout, "mystringcheckvalidation is: %s \n", elektraGetMystringcheckvalidation(elektra));
    fprintf(stdout, "myunsignedshortcheckrange is: %d \n", elektraGetMyunsignedshortcheckrange(elektra));
    fprintf(stdout, "elektraGet...() was executed for all keys available in specification!\n");
}

4. Link and compile

cc -g -O0 myapp.c conf.c `pkg-config --cflags --libs elektra-codegen` -I. -o myapp -Wl,-rpath `pkg-config --variable=libdir elektra-codegen`

5. Mount specification file and spec-mount via generated conf.mount.sh:
   $ APP_PATH=`pwd`/myapp sh conf.mount.sh

Expected vs. actual results

kdb set - works

When kdb set is used all three classes of validation errors are properly detected. No examples here for sake of brevity.

CLI options - don't work

When the keys are set by passing CLI options to myapp, the validation errors are not detected. The actual behavior depends on the class of the error.

Class 1: missing argument

1. Execute ./myapp --mb (--mb can be replaced by any other option, e.g. --me)

Expected:
loadConfiguration () fails with error message "option --mb requires an argument!"

Actual:
Missing value ignored, default value applied.

myboolean is: 1 
myenum is: 0 
myfloatcheckrange is: 1.000000 
mystringcheckdate is: 1:0 
mystringcheckvalidation is: aaa 
myunsignedshortcheckrange is: 9 
elektraGet...() was executed for all keys available in specification!

Class 2: wrong type

1. Execute ./myapp --mb notabool
2. Execute ./myapp --mfcr notafloat
3. Execute ./myapp --muscr notanunsignedshort

Expected:
loadConfiguration () fails with error message "type of OPTION wrong"

Actual:
Application crashes within elektraGetMyboolean() (or the other elektraGet...()s) without an error message. Exit code is -1.

Class 3: validation errors reported by check/* plugins

The check/* plugins seem not to be executed. Their validation is not executed.

1. Execute ./myapp --me illegalenumvalue
2. Execute ./myapp --mfcr 99.0
3. Execute ./myapp --mscd notaiso8601date
4. Execute ./myapp --mscv notthestring-aaa
5. Execute ./myapp --muscr 999

Expected:
loadConfiguration () fails with error message "Validation error for OPTION, reason: ..."

Actual:
Application crashes within elektraGetMyenum() (or the other elektraGet...()s) without an error message. Exit code is -1.

System Information

• Elektra Version: 0.9.7
• Operating System: Ubuntu 20.04
• Versions of other relevant software: n/a

[markus2330]

@kodebach can this be solved by changing the placement of gopts or is this a bigger problem?

[kodebach]

can this be solved by changing the placement of gopts

gopts should already be called before e.g. type. That's why the procgetstorage position exists.

Steps to Reproduce the Problem

Did you mount the spec and then spec-mount correctly? You didn't list this in your reproduction. Without spec-mount plugins like type wouldn't be mounted. This would explain class 2 and 3.

Class 1: missing argument

This would be a bug in elektraGetOpts. It should report an error, which in turn should be reported by gopts and then by kdbGet, and finally by elektraOpen/loadConfiguration.

gopts should be mounted via the contract even without spec-mount. But if the specification itself isn't mounted, then gopts would be a no-op.

Application crashes [...] without an error message. Exit code is -1.

That's the default error handler, you can set a different one with elektraFatalErrorHandler. But probably you won't get much more information in this case. I'm pretty sure that the type plugin didn't run and elektraGet* tried to read an incompatible value

[qwepoizt]

@kodebach Thanks for your comment! I think your last paragraph was accidentally cut off? It ends with

I'm pretty sure that

Adding elektraFatalErrorHandler()

Ah, thanks for the hint! I've now added

static void onFatalError(ElektraError * error) {
    fprintf(stderr, "An error occured during an Elektra operation: %s \n", elektraErrorDescription(error));
    elektraErrorReset(&error);
    exit(EXIT_FAILURE);
}

and put elektraFatalErrorHandler (elektra, onFatalError); after the two if (rc == ...) blocks.

That changes the actual behavior:

Class 2: wrong type

Now onFatalError() is called during the problematic elektraGet...() call.

E.g. for ./myapp --mb notabool the output is now:
An error occured during an Elektra operation: The value 'notabool' of key 'myboolean' could not be converted to type 'boolean'.

E.g. for ./myapp --mb notabool the output is now:
An error occured during an Elektra operation: The value 'notabool' of key 'myboolean' could not be converted to type 'boolean'.

That is still not the expected behavior, because these problems should already be detected during loadConfiguration(). During elektraGet...() it is too late.

Class 3: validation errors reported by check/* plugins

The behavior is unchanged. However, I made a mistake in my original report. The application only crashes for illegal enum or boolean values. So providing wrong values for these 2 types is actually error class 1 2: wrong type.

For other types it does not crash, when the check/* plugins' rules are violated. Instead the elektraGet...() calls return the invalid data without an error.

E.g. ./myapp --mfcr 99.0 --mscd notaiso8601date --mscv notthestring-aaa --muscr 999 yields:

myboolean is: 1 
myenum is: 0 
myfloatcheckrange is: 99.000000 
mystringcheckdate is: notaiso8601date 
mystringcheckvalidation is: notthestring-aaa 
myunsignedshortcheckrange is: 999 
elektraGet...() was executed for all keys available in specification!

Mountpoints, loaded plugins

Did you mount the spec and then spec-mount correctly?

Yes, but I did forgot to document it in my report above :). I've now added step 5. above.

After step 5. the following output occurs for:

kdb mount:

test.ecf on /sw/example/myapp/#0/current with name /sw/example/myapp/#0/current
myapp.overlay.spec.eqd on spec:/sw/example/myapp/#0/current with name spec:/sw/example/myapp/#0/current

kdb ls system:/elektra/mountpoints/

system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/config
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/config/fcrypt/textmode
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/config/path
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/errorplugins
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/errorplugins/#5#resolver_fm_hpu_b#resolver#
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/getplugins
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/getplugins/#0#resolver
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/getplugins/#5#dump#storage#
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/getplugins/#7#range#range#
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/getplugins/#8#type#type#
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/getplugins/#9#date#date#
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/mountpoint
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/setplugins
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/setplugins/#0#resolver
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/setplugins/#1#date
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/setplugins/#2#type
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/setplugins/#3#range
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/setplugins/#4#validation#validation#
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/setplugins/#5#storage
system:/elektra/mountpoints/\/sw\/example\/myapp\/#0\/current/setplugins/#7#resolver
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/config
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/config/path
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/errorplugins
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/errorplugins/#5#noresolver#noresolver#
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/getplugins
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/getplugins/#0#noresolver
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/getplugins/#5#specload#specload#
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/getplugins/#5#specload#specload#/config
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/getplugins/#5#specload#specload#/config/app
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/mountpoint
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/setplugins
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/setplugins/#0#noresolver
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/setplugins/#5#specload
system:/elektra/mountpoints/spec:\/sw\/example\/myapp\/#0\/current/setplugins/#6#sync#sync#

Summary

• Class 1 error is an error within (g)opts. I have an idea how to fix it.
• For Class 2 and 3 errors, I am not sure where to start.
  • Does the output above mean the check/* plugins are loaded?
  • Could it be that the gopts plugin has a lower position during execution of myapp, sets the invalid keys and therefore the check/* plugins have no chance to report an error?
  • The file myapp.overlay.spec.eqd does not actually exist on disk. Could that be the cause?
  • @kodebach Do you have a suggestion on how to proceed troubleshooting here?

[kodebach]

I think your last paragraph was accidentally cut off?

Oh sorry, it should read: "I'm pretty sure that the type plugin didn't run and elektraGet* tried to read an incompatible value". Which is exactly what happend.

The file myapp.overlay.spec.eqd does not actually exist on disk. Could that be the cause?

No that's mostly normal with specload. But you could try the option without specload, maybe specload causes some unexpected problems.

---

Honestly, I have no idea what is going on here. The errors look like type and the other plugins aren't executed. But the mountpoints config contains the plugins, so they should be executed. The only other explanation would be that the spec:/ metakeys aren't copied to the non-spec:/ keys correctly. This could be an error in the spec plugin, or somehow the spec data doesn't get to the spec plugin.

I'll look into this myself and report back, when I know more.

[qwepoizt]

I looked into this and discovered - by reading the code and debugging - that:

• The check/* plugins (e.g. range, validation) only perform their validation during elektraPLUGINNAMESet() and not during elektraPLUGINNAMEGet(). The ...Get() functions pass the unchanged KeySet through.

c.f. range.c:elektraRangeGet():

libelektra/src/plugins/range/range.c

Lines 468 to 488 in 6701d9e

	int elektraRangeGet (Plugin * handle ELEKTRA_UNUSED, KeySet * returned ELEKTRA_UNUSED, Key * parentKey ELEKTRA_UNUSED)
	{
	if (!elektraStrCmp (keyName (parentKey), "system:/elektra/modules/range"))
	{
	KeySet * contract =
	ksNew (30, keyNew ("system:/elektra/modules/range", KEY_VALUE, "range plugin waits for your orders", KEY_END),
	keyNew ("system:/elektra/modules/range/exports", KEY_END),
	keyNew ("system:/elektra/modules/range/exports/get", KEY_FUNC, elektraRangeGet, KEY_END),
	keyNew ("system:/elektra/modules/range/exports/set", KEY_FUNC, elektraRangeSet, KEY_END),
	keyNew ("system:/elektra/modules/range/exports/validateKey", KEY_FUNC, validateKey, KEY_END),
	#include ELEKTRA_README
	keyNew ("system:/elektra/modules/range/infos/version", KEY_VALUE, PLUGINVERSION, KEY_END), KS_END);
	ksAppend (returned, contract);
	ksDel (contract);
	return 1; // success
	}
	// get all keys
	return 1; // success
	}

• The opts library directly writes the parsed CLI options into the proc:/ namespace. It performs no kdbSet().
  c.f. the documentation for opts.c:writeOptionValues()
  

  libelektra/src/libs/opts/opts.c

  Lines 1219 to 1226 in 6701d9e

  	/**
  	* Add keys to the proc namespace in @p ks for all options specified
  	* on @p keyWithOpt. The env-vars are taken from @p envValues.
  	* @retval -1 in case of error
  	* @retval 0 if no key was added
  	* @retval 1 if keys were added to @p ks
  	*/
  	int writeOptionValues (KeySet * ks, Key * keyWithOpt, KeySet * options, Key * errorKey)

• Therefore, the check/* plugins' elektraPLUGINNAMESet() functions are not called during elektraOpen() and no validation is performed during startup of the application using the HL API.

@kodebach

• Could this be the cause?
• Would a sensible fix be, to have the check/* plugins execute the same validation during elektraPLUGINNAMEGet() which is performed during elektraPLUGINNAMESet()?
• If not, do you have a suggestion for a fix?

[kodebach]

Could this be the cause?

Yes, I'm pretty sure this is the cause. I didn't immediately suggest it, because I thought that at least type also does its validation during kdbGet.

Would a sensible fix be, to have the check/* plugins execute the same validation during elektraPLUGINNAMEGet() which is performed during elektraPLUGINNAMESet()?

I have advocated for this multiple times, because it is also a problem, if you modify the config file manually (which will be quite common for some applications). However, until now it seemed I'm the only one that was in favour of enforcing this.

Now that we have a second (more immediate) use-case, we can probably all agree that all validation plugins should also do validation during kdbGet.

If not, do you have a suggestion for a fix?

I don't see any other option, since proc:/ should be ignored during kdbSet. It doesn't make sense to store proc:/ keys anywhere. They are per definition only relevant to the current process.

[qwepoizt]

I see, thank you! @markus2330: What do you think about adding validation to elektraPLUGINNAMEGet() ? Are there sensible alternatives?

…

On 2021-09-13 21:53, kodebach wrote: > Could this be the cause? Yes, I'm pretty sure this is the cause. I didn't immediately suggest it, because I thought that at least type also does its validation during kdbGet. > Would a sensible fix be, to have the check/* plugins execute the > same validation during elektraPLUGINNAMEGet() which is performed > during elektraPLUGINNAMESet()? I have advocated for this multiple times, because it is also a problem, if you modify the config file manually (which will be quite common for some applications). However, until now it seemed I'm the only one that was in favour of enforcing this. > If not, do you have a suggestion for a fix? I don't see any other option, since proc:/ should be ignored during kdbSet. It doesn't make sense to store proc:/ keys anywhere. They are per definition only relevant to the current process. -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4]. [ { ***@***.***": "http://schema.org", ***@***.***": "EmailMessage", "potentialAction": { ***@***.***": "ViewAction", "target": "#4029 (comment)", "url": "#4029 (comment)", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { ***@***.***": "Organization", "name": "GitHub", "url": "https://github.com" } } ] Links: ------ [1] #4029 (comment) [2] https://github.com/notifications/unsubscribe-auth/AOX4AP4H6YPZWAULYC3RVELUBZJD3ANCNFSM5DZSEMBA [3] https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 [4] https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub

[qwepoizt]

Some check/* plugins already have the postgetstorage placement, meaning their elektraPLUGINNAMEGet() is executed and they could execute their validation.

If we decide to execute their validation during elektraPLUGINNAMEGet(), we will also have to make sure that they have the postgetstorage placement!

E.g. check/validation currently does not have postgetstorage thusly its elektraPLUGINNAMEGet() is not even executed during elektraOpen().

[qwepoizt]

Note: I fixed a typo in #4029 (comment). The value of [mystringcheckdate] 's opt/arg was require, now fixed to required.

[qwepoizt]

I've split this up in 3 issues, please let's continue the discussion there:

• Class 1: missing argument (forked to gopts: No error on missing argument  #4042)
• Class 2: wrong type (forked to gopts: Wrong type detected too late #4043)
• Class 3: validation errors reported by check/* plugins (forked to gopts: check/ plugins perform no validation during kdbGet() #4049)

[markus2330]

I've split this up in 3 issues, please let's continue the discussion there:

Can we close this issue then?

[qwepoizt]

I've split this up in 3 issues, please let's continue the discussion there:

Can we close this issue then?

Yes, good idea!