From 4b75da985d3659d70b9016f18f2b8a9b4b068918 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Wed, 24 Jul 2024 09:35:35 -0400 Subject: [PATCH] Fix an error in node ID handling in `crm_node -i` - Resolves: RHEL-49928 --- 012-dont-set-as-xml-id.patch | 121 ++++++++++++++++++++++++++++++++ 013-crm_node-i-initialize.patch | 89 +++++++++++++++++++++++ pacemaker.spec | 8 ++- 3 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 012-dont-set-as-xml-id.patch create mode 100644 013-crm_node-i-initialize.patch diff --git a/012-dont-set-as-xml-id.patch b/012-dont-set-as-xml-id.patch new file mode 100644 index 0000000..cd87944 --- /dev/null +++ b/012-dont-set-as-xml-id.patch @@ -0,0 +1,121 @@ +From d7c233090057d4f660fa458a2ff97896b15ea951 Mon Sep 17 00:00:00 2001 +From: Reid Wahl +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 +--- + 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 + +-#include +-#include + #include ++#include // PRIu32 ++#include ++#include // uint32_t ++#include ++ + #include + + #include +@@ -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); + diff --git a/013-crm_node-i-initialize.patch b/013-crm_node-i-initialize.patch new file mode 100644 index 0000000..770b1ba --- /dev/null +++ b/013-crm_node-i-initialize.patch @@ -0,0 +1,89 @@ +From 22e093a5bff608c86d0ea68588078ca747a6d945 Mon Sep 17 00:00:00 2001 +From: Reid Wahl +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 +--- + 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); + diff --git a/pacemaker.spec b/pacemaker.spec index bec9c46..1ba68c4 100644 --- a/pacemaker.spec +++ b/pacemaker.spec @@ -244,7 +244,7 @@ Name: pacemaker Summary: Scalable High-Availability cluster resource manager Version: %{pcmkversion} -Release: %{pcmk_release}.1%{?dist} +Release: %{pcmk_release}.2%{?dist} %if %{defined _unitdir} License: GPL-2.0-or-later AND LGPL-2.1-or-later %else @@ -276,6 +276,8 @@ Patch008: 008-attrd-prep.patch Patch009: 009-attrd-cache-3.patch Patch010: 010-crm_attribute-free.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: %{pkgname_pcmk_libs}%{?_isa} = %{version}-%{release} @@ -1027,6 +1029,10 @@ exit 0 %license %{nagios_name}-%{nagios_hash}/COPYING %changelog +* Wed Jul 24 2024 Chris Lumens - 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 - 2.1.7-5.1 - Fix a memory leak in the attribute daemon - Resolves: RHEL-40145