Sanitize characters in environment variables #3914
Conversation
|
This does not check for the condition that environment variables cannot start with a number, but that could be added if necessary. Also, it's not clear to me why |
| @@ -928,15 +928,15 @@ crmadmin_node(pcmk__output_t *out, va_list args) | |||
| { | |||
| const char *type = va_arg(args, const char *); | |||
| const char *name = va_arg(args, const char *); | |||
| const char *id = va_arg(args, const char *); | |||
| const char *value = va_arg(args, const char *); | |||
There was a problem hiding this comment.
I don't think this commit clarifies things. It's the "value" only in the context of the "export <name>=<value>" string. In all contexts, it's the node ID. In all other cases, the argument is either unused or treated just as a node ID:
- In this function (default format): used as ID when bash_export=false
- In
crmadmin_node_text(): unused - In
crmadmin_node_xml(): used as ID
| pcmk__s(name, "<null>"), pcmk__s(value, "")); | ||
| int rc = pcmk_rc_ok; | ||
| gchar *replaced = g_strcanon(g_strdup(pcmk__s(name, "<null>")), | ||
| "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_", '_'); |
There was a problem hiding this comment.
Hmm. This is reasonable so that there's no disallowed character in an export command that we output. However, it also defeats the intended purpose. The purpose seems to give commands that make a node's name an alias for its (probably numeric) ID. These export commands are used only to set <node_name>=<node_id> -- no other context. If the name of the environment variable doesn't match the node name, then this seems wrong.
On the other hand, I don't think it's very useful in the first place. Even if the environment variable names match the node names, the user has to copy-paste the environment variable names into their future commands.
So I guess this approach is fine, although I would be very okay with deprecating this functionality if you want.
There was a problem hiding this comment.
Coverity thinks this is leaking memory. That's unfortunate: AFAICT it's totally safe and matches the GLib documentation's usage example. If you agree, then we can either add a suppression or free the g_strdup() result instead of the g_strcanon() result (even though they're the same pointer).
[2025-07-07T19:34:56.483Z] /pacemaker/lib/pacemaker/pcmk_output.c:936:9:
[2025-07-07T19:34:56.483Z] Type: Resource leak (RESOURCE_LEAK)
[2025-07-07T19:34:56.483Z]
[2025-07-07T19:34:56.483Z] /pacemaker/lib/pacemaker/pcmk_output.c:934:5:
[2025-07-07T19:34:56.483Z] 1. path: Condition "bash_export", taking true branch.
[2025-07-07T19:34:56.483Z] /pacemaker/lib/pacemaker/pcmk_output.c:936:9:
[2025-07-07T19:34:56.483Z] 2. alloc_fn: Storage is returned from allocation function "g_strdup_inline".
[2025-07-07T19:34:56.483Z] /usr/include/glib-2.0/glib/gstrfuncs.h:311:3:
[2025-07-07T19:34:56.483Z] 2.1. path: Condition "0", taking false branch.
[2025-07-07T19:34:56.483Z] /usr/include/glib-2.0/glib/gstrfuncs.h:314:3:
[2025-07-07T19:34:56.483Z] 2.2. path: Condition "0", taking false branch.
[2025-07-07T19:34:56.483Z] /usr/include/glib-2.0/glib/gstrfuncs.h:321:3:
[2025-07-07T19:34:56.483Z] 2.3. alloc_fn: Storage is returned from allocation function "g_strdup".
[2025-07-07T19:34:56.483Z] /usr/include/glib-2.0/glib/gstrfuncs.h:321:3:
[2025-07-07T19:34:56.483Z] 2.4. return_alloc_fn: Directly returning storage allocated by "g_strdup".
[2025-07-07T19:34:56.483Z] /pacemaker/lib/pacemaker/pcmk_output.c:936:9:
[2025-07-07T19:34:56.483Z] 3. noescape: Resource "g_strdup_inline(pcmk__s(name, "<null>"))" is not freed or pointed-to in "g_strcanon".
[2025-07-07T19:34:56.483Z] /pacemaker/lib/pacemaker/pcmk_output.c:936:9:
[2025-07-07T19:34:56.483Z] 4. leaked_storage: Failing to save or free storage allocated by "g_strdup_inline(pcmk__s(name, "<null>"))" leaks it.
There was a problem hiding this comment.
We could use
G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "_"
as the set of valid characters, so that it's clear we're not missing anything. (I know that you copy-pasted so it's fine in practice.) I have no strong feelings. Those GLib macros are intended for use with GScannerConfig (for lexical scanning) but they would work in other contexts like this.
There was a problem hiding this comment.
So I guess this approach is fine, although I would be very okay with deprecating this functionality if you want.
We'd need to make sure no one is using it for anything - I guess asking on the users list, and maybe asking the pcs guys would be a good step. But yeah I'm happy with deprecating it too.
This was introduced by 7e50ba6, which of course is not a very enlightening commit.
No description provided.