Skip to content

Sanitize characters in environment variables #3914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+21 −16
Open

Sanitize characters in environment variables #3914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d9df98
Refactor: libpacemaker: Rename id to value in crmadmin_node*.
clumens Jul 7, 2025
57e970a
Low: libpacemaker: Sanitize characters in environment variable names
clumens Jul 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions cts/cli/regression.crmadmin.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,17 @@ overcloud-rabbit-2
=#=#=#= End test: Minimally list all nodes (XML) - OK (0) =#=#=#=
* Passed: crmadmin - Minimally list all nodes (XML)
=#=#=#= Begin test: List all nodes as bash exports =#=#=#=
export overcloud-controller-0=1
export overcloud-controller-1=2
export overcloud-controller-2=3
export overcloud-galera-0=4
export overcloud-galera-1=5
export overcloud-galera-2=6
export overcloud_controller_0=1
export overcloud_controller_1=2
export overcloud_controller_2=3
export overcloud_galera_0=4
export overcloud_galera_1=5
export overcloud_galera_2=6
export lxc1=lxc1
export lxc2=lxc2
export overcloud-rabbit-0=overcloud-rabbit-0
export overcloud-rabbit-1=overcloud-rabbit-1
export overcloud-rabbit-2=overcloud-rabbit-2
export overcloud_rabbit_0=overcloud-rabbit-0
export overcloud_rabbit_1=overcloud-rabbit-1
export overcloud_rabbit_2=overcloud-rabbit-2
=#=#=#= End test: List all nodes as bash exports - OK (0) =#=#=#=
* Passed: crmadmin - List all nodes as bash exports
=#=#=#= Begin test: List cluster nodes =#=#=#=
Expand Down
19 changes: 12 additions & 7 deletions lib/pacemaker/pcmk_output.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,15 +928,20 @@ 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 *);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bool bash_export = va_arg(args, int);

if (bash_export) {
return out->info(out, "export %s=%s",
pcmk__s(name, "<null>"), pcmk__s(id, ""));
int rc = pcmk_rc_ok;
gchar *replaced = g_strcanon(g_strdup(pcmk__s(name, "<null>")),
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_", '_');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


rc = out->info(out, "export %s=%s", replaced, pcmk__s(value, ""));
g_free(replaced);
return rc;
} else {
return out->info(out, "%s node: %s (%s)", type ? type : "cluster",
pcmk__s(name, "<null>"), pcmk__s(id, "<null>"));
pcmk__s(name, "<null>"), pcmk__s(value, "<null>"));
}
}

Expand All @@ -950,7 +955,7 @@ crmadmin_node_text(pcmk__output_t *out, va_list args)
} else {
const char *type G_GNUC_UNUSED = va_arg(args, const char *);
const char *name = va_arg(args, const char *);
const char *id G_GNUC_UNUSED = va_arg(args, const char *);
const char *value G_GNUC_UNUSED = va_arg(args, const char *);
bool bash_export G_GNUC_UNUSED = va_arg(args, int);

pcmk__formatted_printf(out, "%s\n", pcmk__s(name, "<null>"));
Expand All @@ -965,13 +970,13 @@ crmadmin_node_xml(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 *);
bool bash_export G_GNUC_UNUSED = va_arg(args, int);

pcmk__output_create_xml_node(out, PCMK_XE_NODE,
PCMK_XA_TYPE, pcmk__s(type, "cluster"),
PCMK_XA_NAME, pcmk__s(name, ""),
PCMK_XA_ID, pcmk__s(id, ""),
PCMK_XA_ID, pcmk__s(value, ""),
NULL);
return pcmk_rc_ok;
}
Expand Down