Libelektra: kdbSet completely breaks with KEY_FLAG_RO_NAME

[kodebach]

TL;DR: kdbSet completely breaks and potentially destroys the KDB, if the parentKey is read-only (KEY_FLAG_RO_NAME).

---

If you call kdbSet with a read-only parentKey, calls to keySetName will not be able to modify the name of parentKey. There are many places, where this is a problem, but this is the most obvious one:

https://github.com/ElektraInitiative/libelektra/blob/d9a8478478b0009971a070d2990af0b310abfd7e/src%2Flibs%2Felektra%2Fkdb.c#L1320

Instead of setting the name to the correct mountpoint for the backend, keySetName will do nothing to parentKey and return -1.

https://github.com/ElektraInitiative/libelektra/blob/4d43a084bccae3897a11a2e018f4984baefce0a9/src%2Flibs%2Felektra%2Fkeyname.c#L457-L460

This of course means, that all storage plugins will be called with the same parentKey no matter which mountpoint. The effects can range from immensely bad to catastrophically destructive.

---

@markus2330 @mpranj

I would have made a PR immediately, but I didn't know which of the following solutions would be best:

1. Swap initialParent and parentKey in kdbSet: Instead of using the keyDuped key as a backup that is restored at the end, we use the keyDuped key as the working key and keyDel it at the end.
2. Create the "working" parentKey completely from scratch with keyNew (keyName (parentKey), KEY_END)
3. Just fail, if parentKey is read-only

[mpranj]

Good that you caught this!

Afaik nothing in the API docs prevents the parentKey from having a readonly name, so I think failing is not the best option.
Options 1 and 2 should be fine afaik, but the errors must be copied to the returned key then.

[markus2330]

It is true that at the moment we do not use the key name for communication but probably we should. As you know, kdbGet might return more keys than were requested. Via the parentKey we could communicate which keys were actually retrieved.

The only "disadvantage" would be if applications use the same parentKey for get&set and modify other keys outside of their parentKey and expect them not to be changed. But this is actually against the principle of Elektra that transient and persistent state should always be kept synchronous.

So the easiest and imho best fix would be to have a pre-condition that the parentKey is not allowed to be RO in any aspect (neither name, value nor meta). In any of these cases errors (INTERFACE_ERROR) should be returned.

I see little value of having the possibility of using a Key from a KeySet as parentKey. But if you think this, or other use cases I did not think of, could be useful, we can further discuss the options.

[kodebach]

(neither name, value nor meta). In any of these cases errors (INTERFACE_ERROR) should be returned.

How are we going to return an error on a key with read-only metadata?

I think we should just add a return code -2 for: handle, ks or parentKey are invalid (i.e. NULL, read-only, etc.)

[markus2330]

You are right, we better not set an error with ro-metadata. But currently we only return -1 on any errors in the whole API. So all situations where we cannot properly return errors, we return only -1 without setting an error.

If handle and ks is 0, we return a proper INTERFACE_ERROR error. This is already handled, so nothing to do there.

[mpranj]

Most important would be to catch this before the storage plugins are called and return with some error.

I don't know if it pays off to add more return values for various errors. But I see that a readonly parentKey (metadata) is a special case where we can't add the error message to the key. @kodebach if you think -2 would be appropriate, then it's definitely good that you caught this pre 1.0.

[markus2330]

There was already a quite lengthy discussion about -2 in #3029 and there it is much more important to actually distinguish about the return values as it is not a simple "you are using the API totally wrong so that we are unable to yield a proper error" (as it would be here) but something that happened in real-world programs from user-input (wrongly typed names).

To not go into deeply involved in discussions without the person who actually will do this across the whole API (Stefan), I would suggest to simply fix this concrete problem KEY_FLAG_RO_NAME with the appropriate INTERFACE_ERROR and carry on with the other important things for 0.9.4.

[kodebach]

I would suggest to simply fix this concrete problem KEY_FLAG_RO_NAME with the appropriate INTERFACE_ERROR and carry on with the other important things for 0.9.4.

Yes, I think this is definitely the way for now. In addition, I think kdbSet should fail immediately (i.e. the first if should check this), if parentKey has KEY_FLAG_RO_META set. That way we guarantee that this is the only case, where we return -1 and do not set an error on parentKey.