From f03fffa7cdb2d6a7035d004d038a6ec0cf17223b Mon Sep 17 00:00:00 2001 From: Pavla Kratochvilova Date: Mon, 30 Mar 2026 12:09:20 +0200 Subject: [PATCH 1/4] comps: Handle errors while serializing environments Based on commits 74706c1 and d1e1e76 which dealt with the groups serialization. By default libxml2 prints errors (e.g. "permission denied") directly to stderr which interferes with dnf5 output. This patch collects all errors to make them part of the exception message later. This also fixes the memory leak that occurs because the `doc` variable is not properly freed before an error is thrown. --- libdnf5/comps/environment/environment.cpp | 34 +++++++++++++++++++++-- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/libdnf5/comps/environment/environment.cpp b/libdnf5/comps/environment/environment.cpp index 6d675c0a6c..039bd8d286 100644 --- a/libdnf5/comps/environment/environment.cpp +++ b/libdnf5/comps/environment/environment.cpp @@ -20,6 +20,7 @@ #include "libdnf5/comps/environment/environment.hpp" #include "solv/pool.hpp" +#include "utils/string.hpp" #include "utils/xml.hpp" #include "libdnf5/base/base.hpp" @@ -267,7 +268,23 @@ bool Environment::get_installed() const { } +// libxml2 error handler. By default libxml2 prints errors directly to stderr which +// makes a mess of the outputs. +// This stores the errors in a vector of strings; +__attribute__((__format__(printf, 2, 0))) static void error_to_strings(void * ctx, const char * fmt, ...) { + auto xml_errors = static_cast *>(ctx); + char buffer[256]; + va_list args; + va_start(args, fmt); + vsnprintf(buffer, 256, fmt, args); + va_end(args); + xml_errors->push_back(buffer); +} + void Environment::serialize(const std::string & path) { + std::vector xml_errors; + xmlSetGenericErrorFunc(&xml_errors, &error_to_strings); + // Create doc with root node "comps" xmlDocPtr doc = xmlNewDoc(BAD_CAST "1.0"); xmlNodePtr node_comps = xmlNewNode(NULL, BAD_CAST "comps"); @@ -348,12 +365,23 @@ void Environment::serialize(const std::string & path) { } // Save the document - if (xmlSaveFormatFileEnc(path.c_str(), doc, "utf-8", 1) == -1) { - throw utils::xml::XMLSaveError(M_("failed to save xml document for comps")); - } + auto save_result = xmlSaveFormatFileEnc(path.c_str(), doc, "utf-8", 1); // Memory free xmlFreeDoc(doc); + // reset the error handler to default + xmlSetGenericErrorFunc(NULL, NULL); + + if (save_result == -1) { + // There can be duplicit messages in the libxml2 errors so make them unique + auto it = unique(xml_errors.begin(), xml_errors.end()); + xml_errors.resize(static_cast(distance(xml_errors.begin(), it))); + throw utils::xml::XMLSaveError( + M_("Failed to save xml document for environment \"{}\" to file \"{}\": {}"), + get_environmentid(), + path, + libdnf5::utils::string::join(xml_errors, ", ")); + } } void Environment::add_environment_id(const EnvironmentId & environment_id) { From 41c857f62160d65b972e2f017850e65722fae157 Mon Sep 17 00:00:00 2001 From: Pavla Kratochvilova Date: Thu, 9 Apr 2026 13:36:39 +0200 Subject: [PATCH 2/4] comps: Make sure the xml error handler is restored when destroyed Also move the code for dealing with xml errors into the utils to be used both by group and environment serialization. --- libdnf5/comps/environment/environment.cpp | 22 ++------------------- libdnf5/comps/group/group.cpp | 23 ++-------------------- libdnf5/utils/xml.cpp | 20 +++++++++++++++++++ libdnf5/utils/xml.hpp | 24 +++++++++++++++++++++++ 4 files changed, 48 insertions(+), 41 deletions(-) diff --git a/libdnf5/comps/environment/environment.cpp b/libdnf5/comps/environment/environment.cpp index 039bd8d286..dbd45fa222 100644 --- a/libdnf5/comps/environment/environment.cpp +++ b/libdnf5/comps/environment/environment.cpp @@ -268,22 +268,9 @@ bool Environment::get_installed() const { } -// libxml2 error handler. By default libxml2 prints errors directly to stderr which -// makes a mess of the outputs. -// This stores the errors in a vector of strings; -__attribute__((__format__(printf, 2, 0))) static void error_to_strings(void * ctx, const char * fmt, ...) { - auto xml_errors = static_cast *>(ctx); - char buffer[256]; - va_list args; - va_start(args, fmt); - vsnprintf(buffer, 256, fmt, args); - va_end(args); - xml_errors->push_back(buffer); -} - void Environment::serialize(const std::string & path) { std::vector xml_errors; - xmlSetGenericErrorFunc(&xml_errors, &error_to_strings); + utils::xml::GenericErrorFuncGuard error_guard(&xml_errors, &utils::xml::error_to_strings); // Create doc with root node "comps" xmlDocPtr doc = xmlNewDoc(BAD_CAST "1.0"); @@ -369,18 +356,13 @@ void Environment::serialize(const std::string & path) { // Memory free xmlFreeDoc(doc); - // reset the error handler to default - xmlSetGenericErrorFunc(NULL, NULL); if (save_result == -1) { - // There can be duplicit messages in the libxml2 errors so make them unique - auto it = unique(xml_errors.begin(), xml_errors.end()); - xml_errors.resize(static_cast(distance(xml_errors.begin(), it))); throw utils::xml::XMLSaveError( M_("Failed to save xml document for environment \"{}\" to file \"{}\": {}"), get_environmentid(), path, - libdnf5::utils::string::join(xml_errors, ", ")); + libdnf5::utils::string::join(utils::xml::make_errors_unique(std::move(xml_errors)), ", ")); } } diff --git a/libdnf5/comps/group/group.cpp b/libdnf5/comps/group/group.cpp index 0bcab0a89a..b825504b77 100644 --- a/libdnf5/comps/group/group.cpp +++ b/libdnf5/comps/group/group.cpp @@ -37,7 +37,6 @@ extern "C" { } #include -#include #include #include @@ -266,22 +265,9 @@ bool Group::get_installed() const { } -// libxml2 error handler. By default libxml2 prints errors directly to stderr which -// makes a mess of the outputs. -// This stores the errors in a vector of strings; -__attribute__((__format__(printf, 2, 0))) static void error_to_strings(void * ctx, const char * fmt, ...) { - auto xml_errors = static_cast *>(ctx); - char buffer[256]; - va_list args; - va_start(args, fmt); - vsnprintf(buffer, 256, fmt, args); - va_end(args); - xml_errors->push_back(buffer); -} - void Group::serialize(const std::string & path) { std::vector xml_errors; - xmlSetGenericErrorFunc(&xml_errors, &error_to_strings); + utils::xml::GenericErrorFuncGuard error_guard(&xml_errors, &utils::xml::error_to_strings); // Create doc with root node "comps" xmlDocPtr doc = xmlNewDoc(BAD_CAST "1.0"); @@ -363,18 +349,13 @@ void Group::serialize(const std::string & path) { // Memory free xmlFreeDoc(doc); - // reset the error handler to default - xmlSetGenericErrorFunc(NULL, NULL); if (save_result == -1) { - // There can be duplicit messages in the libxml2 errors so make them unique - auto it = unique(xml_errors.begin(), xml_errors.end()); - xml_errors.resize(static_cast(distance(xml_errors.begin(), it))); throw utils::xml::XMLSaveError( M_("Failed to save xml document for group \"{}\" to file \"{}\": {}"), get_groupid(), path, - libdnf5::utils::string::join(xml_errors, ", ")); + libdnf5::utils::string::join(utils::xml::make_errors_unique(std::move(xml_errors)), ", ")); } } diff --git a/libdnf5/utils/xml.cpp b/libdnf5/utils/xml.cpp index 945e8e7a4e..3003415ac3 100644 --- a/libdnf5/utils/xml.cpp +++ b/libdnf5/utils/xml.cpp @@ -22,12 +22,32 @@ #include +#include #include namespace libdnf5::utils::xml { +__attribute__((__format__(printf, 2, 0))) void error_to_strings(void * ctx, const char * fmt, ...) { + auto xml_errors = static_cast *>(ctx); + char buffer[256]; + va_list args; + va_start(args, fmt); + vsnprintf(buffer, 256, fmt, args); + va_end(args); + xml_errors->push_back(buffer); +} + + +std::vector make_errors_unique(std::vector xml_errors) { + std::sort(xml_errors.begin(), xml_errors.end()); + auto it = std::unique(xml_errors.begin(), xml_errors.end()); + xml_errors.resize(static_cast(std::distance(xml_errors.begin(), it))); + return xml_errors; +} + + xmlNodePtr add_subnode_with_text(xmlNodePtr parent, std::string child_name, std::string child_text) { xmlNodePtr node = xmlNewNode(NULL, BAD_CAST child_name.c_str()); xmlAddChild(parent, node); diff --git a/libdnf5/utils/xml.hpp b/libdnf5/utils/xml.hpp index 165fbb1ab3..7caa9adf2d 100644 --- a/libdnf5/utils/xml.hpp +++ b/libdnf5/utils/xml.hpp @@ -23,8 +23,10 @@ #include "libdnf5/common/exception.hpp" #include +#include #include +#include namespace libdnf5::utils::xml { @@ -36,6 +38,28 @@ struct XMLSaveError : public Error { }; +/// Restore the default generic error handler when destroyed. +struct GenericErrorFuncGuard { + GenericErrorFuncGuard(void * ctx, xmlGenericErrorFunc handler) noexcept { xmlSetGenericErrorFunc(ctx, handler); } + ~GenericErrorFuncGuard() { xmlSetGenericErrorFunc(NULL, NULL); } + + GenericErrorFuncGuard(const GenericErrorFuncGuard &) = delete; + GenericErrorFuncGuard & operator=(const GenericErrorFuncGuard &) = delete; + GenericErrorFuncGuard(GenericErrorFuncGuard &&) = delete; + GenericErrorFuncGuard & operator=(GenericErrorFuncGuard &&) = delete; +}; + + +// libxml2 error handler. By default libxml2 prints errors directly to stderr which +// makes a mess of the outputs. +// This stores the errors in a vector of strings. +__attribute__((__format__(printf, 2, 0))) void error_to_strings(void * ctx, const char * fmt, ...); + + +// There can be duplicate messages in the libxml2 errors, so make them unique. +std::vector make_errors_unique(std::vector xml_errors); + + xmlNodePtr add_subnode_with_text(xmlNodePtr parent, std::string child_name, std::string child_text); } // namespace libdnf5::utils::xml From 5bcd614fedf3aa4ec01f6becbad8a949b3882e19 Mon Sep 17 00:00:00 2001 From: Pavla Kratochvilova Date: Thu, 9 Apr 2026 13:48:25 +0200 Subject: [PATCH 3/4] comps: Make sure the doc is always freed at the end of serialization --- libdnf5/comps/environment/environment.cpp | 12 ++++-------- libdnf5/comps/group/group.cpp | 12 ++++-------- libdnf5/utils/xml.hpp | 9 +++++++++ 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/libdnf5/comps/environment/environment.cpp b/libdnf5/comps/environment/environment.cpp index dbd45fa222..50ee4bf07a 100644 --- a/libdnf5/comps/environment/environment.cpp +++ b/libdnf5/comps/environment/environment.cpp @@ -39,6 +39,7 @@ extern "C" { #include #include +#include #include #include #include @@ -273,9 +274,9 @@ void Environment::serialize(const std::string & path) { utils::xml::GenericErrorFuncGuard error_guard(&xml_errors, &utils::xml::error_to_strings); // Create doc with root node "comps" - xmlDocPtr doc = xmlNewDoc(BAD_CAST "1.0"); + std::unique_ptr doc(xmlNewDoc(BAD_CAST "1.0")); xmlNodePtr node_comps = xmlNewNode(NULL, BAD_CAST "comps"); - xmlDocSetRootElement(doc, node_comps); + xmlDocSetRootElement(doc.get(), node_comps); // Create "environment" node xmlNodePtr node_environment = xmlNewNode(NULL, BAD_CAST "environment"); @@ -352,12 +353,7 @@ void Environment::serialize(const std::string & path) { } // Save the document - auto save_result = xmlSaveFormatFileEnc(path.c_str(), doc, "utf-8", 1); - - // Memory free - xmlFreeDoc(doc); - - if (save_result == -1) { + if (xmlSaveFormatFileEnc(path.c_str(), doc.get(), "utf-8", 1) == -1) { throw utils::xml::XMLSaveError( M_("Failed to save xml document for environment \"{}\" to file \"{}\": {}"), get_environmentid(), diff --git a/libdnf5/comps/group/group.cpp b/libdnf5/comps/group/group.cpp index b825504b77..b9b74cab17 100644 --- a/libdnf5/comps/group/group.cpp +++ b/libdnf5/comps/group/group.cpp @@ -39,6 +39,7 @@ extern "C" { #include #include +#include #include #include #include @@ -270,9 +271,9 @@ void Group::serialize(const std::string & path) { utils::xml::GenericErrorFuncGuard error_guard(&xml_errors, &utils::xml::error_to_strings); // Create doc with root node "comps" - xmlDocPtr doc = xmlNewDoc(BAD_CAST "1.0"); + std::unique_ptr doc(xmlNewDoc(BAD_CAST "1.0")); xmlNodePtr node_comps = xmlNewNode(NULL, BAD_CAST "comps"); - xmlDocSetRootElement(doc, node_comps); + xmlDocSetRootElement(doc.get(), node_comps); // Create "group" node xmlNodePtr node_group = xmlNewNode(NULL, BAD_CAST "group"); @@ -345,12 +346,7 @@ void Group::serialize(const std::string & path) { } // Save the document - auto save_result = xmlSaveFormatFileEnc(path.c_str(), doc, "utf-8", 1); - - // Memory free - xmlFreeDoc(doc); - - if (save_result == -1) { + if (xmlSaveFormatFileEnc(path.c_str(), doc.get(), "utf-8", 1) == -1) { throw utils::xml::XMLSaveError( M_("Failed to save xml document for group \"{}\" to file \"{}\": {}"), get_groupid(), diff --git a/libdnf5/utils/xml.hpp b/libdnf5/utils/xml.hpp index 7caa9adf2d..7576601933 100644 --- a/libdnf5/utils/xml.hpp +++ b/libdnf5/utils/xml.hpp @@ -60,6 +60,15 @@ __attribute__((__format__(printf, 2, 0))) void error_to_strings(void * ctx, cons std::vector make_errors_unique(std::vector xml_errors); +struct XmlDocDeleter { + void operator()(xmlDoc * doc) const noexcept { + if (doc != nullptr) { + xmlFreeDoc(doc); + } + } +}; + + xmlNodePtr add_subnode_with_text(xmlNodePtr parent, std::string child_name, std::string child_text); } // namespace libdnf5::utils::xml From 60622f0b48cfd523557fd2fe55518fbd9d415bb8 Mon Sep 17 00:00:00 2001 From: Pavla Kratochvilova Date: Thu, 9 Apr 2026 14:10:38 +0200 Subject: [PATCH 4/4] comps: Throw bad_alloc when memory allocation fails in a libxml function --- libdnf5/comps/environment/environment.cpp | 23 +++++++------ libdnf5/comps/group/group.cpp | 21 ++++++------ libdnf5/utils/xml.cpp | 39 ++++++++++++++++++++--- libdnf5/utils/xml.hpp | 11 ++++++- 4 files changed, 70 insertions(+), 24 deletions(-) diff --git a/libdnf5/comps/environment/environment.cpp b/libdnf5/comps/environment/environment.cpp index 50ee4bf07a..b10010fa77 100644 --- a/libdnf5/comps/environment/environment.cpp +++ b/libdnf5/comps/environment/environment.cpp @@ -275,12 +275,15 @@ void Environment::serialize(const std::string & path) { // Create doc with root node "comps" std::unique_ptr doc(xmlNewDoc(BAD_CAST "1.0")); - xmlNodePtr node_comps = xmlNewNode(NULL, BAD_CAST "comps"); + if (!doc) { + throw std::bad_alloc(); + } + xmlNodePtr node_comps = utils::xml::new_node("comps"); xmlDocSetRootElement(doc.get(), node_comps); // Create "environment" node - xmlNodePtr node_environment = xmlNewNode(NULL, BAD_CAST "environment"); - xmlAddChild(node_comps, node_environment); + xmlNodePtr node_environment = utils::xml::new_node("environment"); + utils::xml::add_child(node_comps, node_environment); // Add id, name, description, display_order utils::xml::add_subnode_with_text(node_environment, "id", get_environmentid()); @@ -316,7 +319,7 @@ void Environment::serialize(const std::string & path) { // If it's successful (wasn't already present), create an XML node for this translation if (name_langs.insert(lang).second) { node = utils::xml::add_subnode_with_text(node_environment, "name", std::string(di.kv.str)); - xmlNewProp(node, BAD_CAST "xml:lang", BAD_CAST lang.c_str()); + utils::xml::new_prop(node, "xml:lang", lang); } } // If keyname starts with "solvable:description:", it's a description translation @@ -326,7 +329,7 @@ void Environment::serialize(const std::string & path) { // If it's successful (wasn't already present), create an XML node for this translation if (description_langs.insert(lang).second) { node = utils::xml::add_subnode_with_text(node_environment, "description", std::string(di.kv.str)); - xmlNewProp(node, BAD_CAST "xml:lang", BAD_CAST lang.c_str()); + utils::xml::new_prop(node, "xml:lang", lang); } } } @@ -334,18 +337,18 @@ void Environment::serialize(const std::string & path) { } // Add grouplist - xmlNodePtr node_grouplist = xmlNewNode(NULL, BAD_CAST "grouplist"); - xmlAddChild(node_environment, node_grouplist); + xmlNodePtr node_grouplist = utils::xml::new_node("grouplist"); + utils::xml::add_child(node_environment, node_grouplist); for (const auto & group : get_groups()) { // Create an XML node for this group node = utils::xml::add_subnode_with_text(node_grouplist, "groupid", group); } - xmlNodePtr node_optionlist = xmlNewNode(NULL, BAD_CAST "optionlist"); - xmlAddChild(node_environment, node_optionlist); + xmlNodePtr node_optionlist = utils::xml::new_node("optionlist"); + utils::xml::add_child(node_environment, node_optionlist); for (const auto & group : get_default_groups()) { // Create an XML node for this group node = utils::xml::add_subnode_with_text(node_optionlist, "groupid", group); - xmlNewProp(node, BAD_CAST "default", BAD_CAST "true"); + utils::xml::new_prop(node, "default", "true"); } for (const auto & group : get_optional_groups()) { // Create an XML node for this group diff --git a/libdnf5/comps/group/group.cpp b/libdnf5/comps/group/group.cpp index b9b74cab17..f78a17cc71 100644 --- a/libdnf5/comps/group/group.cpp +++ b/libdnf5/comps/group/group.cpp @@ -272,12 +272,15 @@ void Group::serialize(const std::string & path) { // Create doc with root node "comps" std::unique_ptr doc(xmlNewDoc(BAD_CAST "1.0")); - xmlNodePtr node_comps = xmlNewNode(NULL, BAD_CAST "comps"); + if (!doc) { + throw std::bad_alloc(); + } + xmlNodePtr node_comps = utils::xml::new_node("comps"); xmlDocSetRootElement(doc.get(), node_comps); // Create "group" node - xmlNodePtr node_group = xmlNewNode(NULL, BAD_CAST "group"); - xmlAddChild(node_comps, node_group); + xmlNodePtr node_group = utils::xml::new_node("group"); + utils::xml::add_child(node_comps, node_group); // Add id, name, description, default, uservisible, display_order, langonly utils::xml::add_subnode_with_text(node_group, "id", get_groupid()); @@ -316,7 +319,7 @@ void Group::serialize(const std::string & path) { // If it's successful (wasn't already present), create an XML node for this translation if (name_langs.insert(lang).second) { node = utils::xml::add_subnode_with_text(node_group, "name", std::string(di.kv.str)); - xmlNewProp(node, BAD_CAST "xml:lang", BAD_CAST lang.c_str()); + utils::xml::new_prop(node, "xml:lang", lang); } } // If keyname starts with "solvable:description:", it's a description translation @@ -326,7 +329,7 @@ void Group::serialize(const std::string & path) { // If it's successful (wasn't already present), create an XML node for this translation if (description_langs.insert(lang).second) { node = utils::xml::add_subnode_with_text(node_group, "description", std::string(di.kv.str)); - xmlNewProp(node, BAD_CAST "xml:lang", BAD_CAST lang.c_str()); + utils::xml::new_prop(node, "xml:lang", lang); } } } @@ -334,14 +337,14 @@ void Group::serialize(const std::string & path) { } // Add packagelist - xmlNodePtr node_packagelist = xmlNewNode(NULL, BAD_CAST "packagelist"); - xmlAddChild(node_group, node_packagelist); + xmlNodePtr node_packagelist = utils::xml::new_node("packagelist"); + utils::xml::add_child(node_group, node_packagelist); for (const auto & pkg : get_packages()) { // Create an XML node for this package node = utils::xml::add_subnode_with_text(node_packagelist, "packagereq", pkg.get_name()); - xmlNewProp(node, BAD_CAST "type", BAD_CAST pkg.get_type_string().c_str()); + utils::xml::new_prop(node, "type", pkg.get_type_string()); if (pkg.get_type() == PackageType::CONDITIONAL) { - xmlNewProp(node, BAD_CAST "requires", BAD_CAST pkg.get_condition().c_str()); + utils::xml::new_prop(node, "requires", pkg.get_condition()); } } diff --git a/libdnf5/utils/xml.cpp b/libdnf5/utils/xml.cpp index 3003415ac3..23b4432a8c 100644 --- a/libdnf5/utils/xml.cpp +++ b/libdnf5/utils/xml.cpp @@ -48,10 +48,41 @@ std::vector make_errors_unique(std::vector xml_errors) } -xmlNodePtr add_subnode_with_text(xmlNodePtr parent, std::string child_name, std::string child_text) { - xmlNodePtr node = xmlNewNode(NULL, BAD_CAST child_name.c_str()); - xmlAddChild(parent, node); - xmlAddChild(node, xmlNewText(BAD_CAST child_text.c_str())); +xmlNodePtr new_node(const std::string & node_name) { + xmlNodePtr node = xmlNewNode(NULL, BAD_CAST node_name.c_str()); + if (!node) { + throw std::bad_alloc(); + } + return node; +} + + +xmlAttrPtr new_prop(xmlNodePtr node, const std::string & name, const std::string & value) { + xmlAttrPtr prop = xmlNewProp(node, BAD_CAST name.c_str(), BAD_CAST value.c_str()); + if (!prop) { + throw std::bad_alloc(); + } + return prop; +} + + +xmlNodePtr add_child(xmlNodePtr parent, xmlNodePtr child) { + libdnf_assert(parent && child && parent != child, "Invalid parent or child node"); + if (!xmlAddChild(parent, child)) { + throw std::bad_alloc(); + } + return child; +} + + +xmlNodePtr add_subnode_with_text(xmlNodePtr parent, const std::string & child_name, const std::string & child_text) { + xmlNodePtr node = new_node(child_name); + add_child(parent, node); + xmlNodePtr text = xmlNewText(BAD_CAST child_text.c_str()); + if (!text) { + throw std::bad_alloc(); + } + add_child(node, text); return node; } diff --git a/libdnf5/utils/xml.hpp b/libdnf5/utils/xml.hpp index 7576601933..cc84db651c 100644 --- a/libdnf5/utils/xml.hpp +++ b/libdnf5/utils/xml.hpp @@ -69,7 +69,16 @@ struct XmlDocDeleter { }; -xmlNodePtr add_subnode_with_text(xmlNodePtr parent, std::string child_name, std::string child_text); +xmlNodePtr new_node(const std::string & node_name); + + +xmlAttrPtr new_prop(xmlNodePtr node, const std::string & name, const std::string & value); + + +xmlNodePtr add_child(xmlNodePtr parent, xmlNodePtr child); + + +xmlNodePtr add_subnode_with_text(xmlNodePtr parent, const std::string & child_name, const std::string & child_text); } // namespace libdnf5::utils::xml