rpms/pacemaker
7
0
Fork 0
Code Issues Pull Requests Releases Activity

Fix an error in node ID handling in crm_node -i

Browse Source 
- Resolves: RHEL-49928
This commit is contained in:
Chris Lumens 2024-07-24 09:35:35 -04:00
parent f19afa96ca
commit 4b75da985d
3 changed files with 217 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

121
012-dont-set-as-xml-id.patch Normal file
View File

@ -0,0 +1,121 @@
From d7c233090057d4f660fa458a2ff97896b15ea951 Mon Sep 17 00:00:00 2001
From: Reid Wahl <nrwahl@protonmail.com>
Date: Thu, 11 Jul 2024 12:43:49 -0700
Subject: [PATCH] Refactor: various: Don't set cluster-layer node ID as XML ID
Currently, we call the pcmk__xe_set_id() function using a stringified
version of the numeric cluster-layer node ID. However, pcmk__xe_set_id()
tries to sanitize its input to a valid XML ID. An XML ID cannot begin
with a digit.
crm_xml_set_id() does not sanitize comprehensively, and in particular,
it does not care whether its argument begins with a digit. So the
current code doesn't cause a problem.
Still, as a best practice, set the PCMK_XA_ID attribute using
crm_xml_add_ll() instead.
Ref T848
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
---
daemons/controld/controld_messages.c | 6 +++++-
lib/cluster/corosync.c | 2 +-
lib/common/ipc_client.c | 2 +-
lib/common/ipc_controld.c | 9 ++++++---
tools/crm_node.c | 4 ++--
5 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/daemons/controld/controld_messages.c b/daemons/controld/controld_messages.c
index bd5237e..0b0f25b 100644
--- a/daemons/controld/controld_messages.c
+++ b/daemons/controld/controld_messages.c
@@ -893,7 +893,11 @@ handle_node_info_request(const xmlNode *msg)
pcmk_is_set(controld_globals.flags,
controld_has_quorum));
- // Check whether client requested node info by ID and/or name
+ /* Check whether client requested node info by ID and/or name
+ *
+ * @TODO A Corosync-layer node ID is of type uint32_t. We should be able to
+ * handle legitimate node IDs greater than INT_MAX, but currently we do not.
+ */
crm_element_value_int(msg, XML_ATTR_ID, &node_id);
if (node_id < 0) {
node_id = 0;
diff --git a/lib/cluster/corosync.c b/lib/cluster/corosync.c
index 374250f..fc33cce 100644
--- a/lib/cluster/corosync.c
+++ b/lib/cluster/corosync.c
@@ -650,7 +650,7 @@ pcmk__corosync_add_nodes(xmlNode *xml_parent)
if (xml_parent) {
xmlNode *node = create_xml_node(xml_parent, XML_CIB_TAG_NODE);
- crm_xml_set_id(node, "%u", nodeid);
+ crm_xml_add_ll(node, XML_ATTR_ID, (long long) nodeid);
crm_xml_add(node, XML_ATTR_UNAME, name);
}
}
diff --git a/lib/common/ipc_client.c b/lib/common/ipc_client.c
index 5e64e23..7696a69 100644
--- a/lib/common/ipc_client.c
+++ b/lib/common/ipc_client.c
@@ -769,7 +769,7 @@ create_purge_node_request(const pcmk_ipc_api_t *api, const char *node_name,
request = create_request(CRM_OP_RM_NODE_CACHE, NULL, NULL,
pcmk_ipc_name(api, false), client, NULL);
if (nodeid > 0) {
- crm_xml_set_id(request, "%lu", (unsigned long) nodeid);
+ crm_xml_add_ll(request, XML_ATTR_ID, (unsigned long) nodeid);
}
crm_xml_add(request, XML_ATTR_UNAME, node_name);
break;
diff --git a/lib/common/ipc_controld.c b/lib/common/ipc_controld.c
index 8e2016e..e4284f5 100644
--- a/lib/common/ipc_controld.c
+++ b/lib/common/ipc_controld.c
@@ -9,9 +9,12 @@
#include <crm_internal.h>
-#include <stdio.h>
-#include <stdbool.h>
#include <errno.h>
+#include <inttypes.h> // PRIu32
+#include <stdbool.h>
+#include <stdint.h> // uint32_t
+#include <stdio.h>
+
#include <libxml/tree.h>
#include <crm/crm.h>
@@ -412,7 +415,7 @@ pcmk_controld_api_node_info(pcmk_ipc_api_t *api, uint32_t nodeid)
return EINVAL;
}
if (nodeid > 0) {
- crm_xml_set_id(request, "%lu", (unsigned long) nodeid);
+ crm_xml_add_ll(request, XML_ATTR_ID, (unsigned long) nodeid);
}
rc = send_controller_request(api, request, true);
diff --git a/tools/crm_node.c b/tools/crm_node.c
index 1e7ce6c..ad8c459 100644
--- a/tools/crm_node.c
+++ b/tools/crm_node.c
@@ -552,7 +552,7 @@ remove_from_section(cib_t *cib, const char *element, const char *section,
}
crm_xml_add(xml, XML_ATTR_UNAME, node_name);
if (node_id > 0) {
- crm_xml_set_id(xml, "%ld", node_id);
+ crm_xml_add_ll(xml, XML_ATTR_ID, node_id);
}
rc = cib->cmds->remove(cib, section, xml, cib_transaction);
free_xml(xml);
@@ -691,7 +691,7 @@ purge_node_from_fencer(const char *node_name, long node_id)
cmd = create_request(CRM_OP_RM_NODE_CACHE, NULL, NULL, "stonith-ng",
crm_system_name, NULL);
if (node_id > 0) {
- crm_xml_set_id(cmd, "%ld", node_id);
+ crm_xml_add_ll(cmd, XML_ATTR_ID, node_id);
}
crm_xml_add(cmd, XML_ATTR_UNAME, node_name);

89
013-crm_node-i-initialize.patch Normal file
View File

@ -0,0 +1,89 @@
From 22e093a5bff608c86d0ea68588078ca747a6d945 Mon Sep 17 00:00:00 2001
From: Reid Wahl <nrwahl@protonmail.com>
Date: Thu, 11 Jul 2024 12:29:34 -0700
Subject: [PATCH] Fix: tools: crm_node -i must initialize nodeid before passing
pointer
This is a regression introduced in 2.1.7 via a27f099.
Currently, crm_node -i passes a pointer to the uninitialized uint32_t
nodeid variable, to pcmk__query_node_info(). Since the pointer is
non-NULL, pcmk__query_node_info() dereferences it. Whatever garbage
value resides there gets passed as the ID to query.
The controller parses the node ID from the request as an int. If the
garbage value is greater than INT_MAX, it overflows to a negative int
value, and the controller (in handle_node_info_request()) defaults it to
0. In that case, there's no problem: we search for the local node name
instead of the garbage node ID.
If the garbage value is less than or equal to INT_MAX, we search for it
directly. We won't find a matching node unless one happens to exist with
that garbage node ID. In the case of no match, crm_node -i outputs "Node
is not known to cluster" instead of the local node's cluster-layer ID.
Thanks to Artur Novik for the report:
https://lists.clusterlabs.org/pipermail/users/2024-July/036270.html
Fixes T847
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
---
lib/pacemaker/pcmk_cluster_queries.c | 22 +++++++++++-----------
tools/crm_node.c | 2 +-
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/lib/pacemaker/pcmk_cluster_queries.c b/lib/pacemaker/pcmk_cluster_queries.c
index 3229fae3eff..8404584580e 100644
--- a/lib/pacemaker/pcmk_cluster_queries.c
+++ b/lib/pacemaker/pcmk_cluster_queries.c
@@ -586,25 +586,25 @@ pcmk_designated_controller(xmlNodePtr *xml, unsigned int message_timeout_ms)
* the controller
*
* \param[in,out] out Output object
- * \param[in,out] node_id ID of node whose name to get. If \p NULL
- * or 0, get the local node name. If not
- * \p NULL, store the true node ID here on
+ * \param[in,out] node_id ID of node whose info to get. If \p NULL
+ * or 0, get the local node's info. If not
+ * \c NULL, store the true node ID here on
* success.
- * \param[out] node_name If not \p NULL, where to store the node
+ * \param[out] node_name If not \c NULL, where to store the node
* name
- * \param[out] uuid If not \p NULL, where to store the node
+ * \param[out] uuid If not \c NULL, where to store the node
* UUID
- * \param[out] state If not \p NULL, where to store the
+ * \param[out] state If not \c NULL, where to store the
* membership state
- * \param[out] is_remote If not \p NULL, where to store whether the
+ * \param[out] is_remote If not \c NULL, where to store whether the
* node is a Pacemaker Remote node
- * \param[out] have_quorum If not \p NULL, where to store whether the
+ * \param[out] have_quorum If not \c NULL, where to store whether the
* node has quorum
* \param[in] show_output Whether to show the node info
* \param[in] message_timeout_ms How long to wait for a reply from the
- * \p pacemaker-controld API. If 0,
- * \p pcmk_ipc_dispatch_sync will be used.
- * Otherwise, \p pcmk_ipc_dispatch_poll will
+ * \c pacemaker-controld API. If 0,
+ * \c pcmk_ipc_dispatch_sync will be used.
+ * Otherwise, \c pcmk_ipc_dispatch_poll will
* be used.
*
* \return Standard Pacemaker return code
diff --git a/tools/crm_node.c b/tools/crm_node.c
index d4153605a69..8aa8d3d29c7 100644
--- a/tools/crm_node.c
+++ b/tools/crm_node.c
@@ -434,7 +434,7 @@ run_controller_mainloop(void)
static void
print_node_id(void)
{
- uint32_t nodeid;
+ uint32_t nodeid = 0;
int rc = pcmk__query_node_info(out, &nodeid, NULL, NULL, NULL, NULL, NULL,
false, 0);

8
pacemaker.spec
View File

@ -244,7 +244,7 @@
Name: pacemaker Name: pacemaker
Summary: Scalable High-Availability cluster resource manager Summary: Scalable High-Availability cluster resource manager
Version: %{pcmkversion} Version: %{pcmkversion}
Release: %{pcmk_release}.1%{?dist} Release: %{pcmk_release}.2%{?dist}
%if %{defined _unitdir} %if %{defined _unitdir}
License: GPL-2.0-or-later AND LGPL-2.1-or-later License: GPL-2.0-or-later AND LGPL-2.1-or-later
%else %else
@ -276,6 +276,8 @@ Patch008: 008-attrd-prep.patch
Patch009: 009-attrd-cache-3.patch Patch009: 009-attrd-cache-3.patch
Patch010: 010-crm_attribute-free.patch Patch010: 010-crm_attribute-free.patch
Patch011: 011-attrd-memory-leak.patch Patch011: 011-attrd-memory-leak.patch
Patch012: 012-dont-set-as-xml-id.patch
Patch013: 013-crm_node-i-initialize.patch
Requires: resource-agents Requires: resource-agents
Requires: %{pkgname_pcmk_libs}%{?_isa} = %{version}-%{release} Requires: %{pkgname_pcmk_libs}%{?_isa} = %{version}-%{release}
@ -1027,6 +1029,10 @@ exit 0
%license %{nagios_name}-%{nagios_hash}/COPYING %license %{nagios_name}-%{nagios_hash}/COPYING
%changelog %changelog
* Wed Jul 24 2024 Chris Lumens <clumens@redhat.com> - 2.1.7-5.2
- Fix an error in node ID handling in `crm_node -i`
- Resolves: RHEL-49928
* Fri Jun 7 2024 Chris Lumens <clumens@redhat.com> - 2.1.7-5.1 * Fri Jun 7 2024 Chris Lumens <clumens@redhat.com> - 2.1.7-5.1
- Fix a memory leak in the attribute daemon - Fix a memory leak in the attribute daemon
- Resolves: RHEL-40145 - Resolves: RHEL-40145