[PATCH 0/8] Avoid changing const string via strchr() and friends
This stems from the discussion here: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HVMCJ... Green pipeline here: https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/2183169334 Michal Prívozník (8): vircommand: Update documentation to virCommandSetDryRun() virstringtest: Introduce a test for virSkipSpacesBackwards() virfirewalltest: Introduce testIPtablesSetupPrivateChains() virSkipSpacesBackwards: Turn @endp into const iptablesPrivateChainCreate: Rename @tmp variable iptablesPrivateChainCreate: Switch to STRSKIP() iptablesPrivateChainCreate: Avoid modifying const string lib: Avoid changing const strings via strchr() and friends src/interface/interface_backend_udev.c | 2 +- src/libxl/xen_common.c | 10 +- src/libxl/xen_xm.c | 10 +- src/network/meson.build | 1 + src/network/network_iptables.c | 33 ++++--- src/node_device/node_device_udev.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 4 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_nbdkit.c | 2 +- src/rpc/virnetsshsession.c | 5 +- src/storage/storage_util.c | 2 +- src/storage_file/storage_source.c | 4 +- src/util/vircgroup.c | 2 +- src/util/vircommand.c | 10 +- src/util/virfile.c | 2 +- src/util/virstoragefile.c | 4 +- src/util/virstring.c | 20 ++-- src/util/virstring.h | 2 +- src/util/virsysinfo.c | 22 ++--- src/util/virxml.c | 3 +- src/vmware/vmware_conf.c | 2 +- tests/meson.build | 9 +- tests/virfirewalltest.c | 111 ++++++++++++++++++++++ tests/virstringtest.c | 48 ++++++++++ tools/vsh.c | 6 +- 25 files changed, 252 insertions(+), 66 deletions(-) -- 2.51.2
From: Michal Privoznik <mprivozn@redhat.com> Throughout years, virCommandSetDryRun() has gained more functionality and arguments. But: 1) not all arguments are covered in documentation, 2) the example wouldn't even compile. Expand the documentation to reflect current behaviour. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index d9e4c0181f..1390c80a32 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -3184,6 +3184,7 @@ virCommandDryRunTokenFree(virCommandDryRunToken *tok) * @bufArgLinebreaks: add linebreaks after command and every argument or argument pair * @bufCommandStripPath: strip leading paths of command * @callback: callback to process input/output/args + * @opaque: data blob to pass to @callback * * Sometimes it's desired to not actually run given command, but * see its string representation without having to change the @@ -3200,13 +3201,14 @@ virCommandDryRunTokenFree(virCommandDryRunToken *tok) * The strings stored in @buf are escaped for a shell and * separated by a newline. For example: * - * virBuffer buffer = VIR_BUFFER_INITIALIZER; - * virCommandSetDryRun(&buffer); - * + * g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; + * g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); * virCommand *echocmd = virCommandNewArgList("/bin/echo", "Hello world", NULL); + * + * virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, NULL, NULL); * virCommandRun(echocmd, NULL); * - * After this, the @buffer should contain: + * After this, the @cmdbuf should contain: * * /bin/echo 'Hello world'\n * -- 2.51.2
From: Michal Privoznik <mprivozn@redhat.com> The signature and implementation of virSkipSpacesBackwards() is soon about to change. Introduce a test case to make sure its behaviour stays the same. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virstringtest.c | 48 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index f4976890db..a9c8e621ce 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -111,6 +111,51 @@ testStringSortCompare(const void *opaque G_GNUC_UNUSED) } +#define TEST_STR "This is a static string with spaces" +#define TEST_SPACES " " + +static int +testSkipSpacesBackwards(const void *opaque G_GNUC_UNUSED) +{ + const char *str = TEST_STR TEST_SPACES; + char *eol = NULL; + + virSkipSpacesBackwards(str, &eol); + + if (STRNEQ(str, TEST_STR TEST_SPACES)) { + fprintf(stderr, "expected '" TEST_STR TEST_SPACES "' got '%s'\n", str); + return -1; + } + + while (g_ascii_isspace(*eol)) + eol++; + + if (*eol != '\0') { + fprintf(stderr, "expected empty string, got '%s'\n", eol); + return -1; + } + + eol = (char *)str + strlen(TEST_STR); + + virSkipSpacesBackwards(str, &eol); + + if (STRNEQ(str, TEST_STR TEST_SPACES)) { + fprintf(stderr, "expected '" TEST_STR TEST_SPACES "' got '%s'\n", str); + return -1; + } + + if (STRNEQ(eol, TEST_SPACES)) { + fprintf(stderr, "expected empty string, got '%s'\n", eol); + return -1; + } + + return 0; +} + +#undef TEST_SPACES +#undef TEST_STR + + struct stringSearchData { const char *str; const char *regexp; @@ -463,6 +508,9 @@ mymain(void) if (virTestRun("virStringSortCompare", testStringSortCompare, NULL) < 0) ret = -1; + if (virTestRun("virSkipSpacesBackwards", testSkipSpacesBackwards, NULL) < 0) + ret = -1; + #define TEST_SEARCH(s, r, x, n, m, e) \ do { \ struct stringSearchData data = { \ -- 2.51.2
From: Michal Privoznik <mprivozn@redhat.com> When the network driver starts up it may inject some firewall rules (e.g. for a network with NAT). So far, this scenario wasn't covered in our test suite. The reason for adding this test is twofold: the fist, check we add correct rules, the second is to cover iptablesPrivateChainCreate() as its implementation is soon to be changed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/meson.build | 1 + tests/meson.build | 9 +++- tests/virfirewalltest.c | 111 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 1 deletion(-) diff --git a/src/network/meson.build b/src/network/meson.build index 51bbf7063d..7a98974852 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -10,6 +10,7 @@ if host_machine.system() == 'freebsd' network_driver_sources += 'network_pf.c' endif +network_inc_dir = include_directories('.') driver_source_files += files(network_driver_sources) stateful_driver_source_files += files(network_driver_sources) diff --git a/tests/meson.build b/tests/meson.build index bb6ee6b4ee..1f73d6c029 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -259,6 +259,13 @@ if conf.has('WITH_QEMU') domaincapstest_link_whole += [ test_utils_qemu_lib ] endif +virfirewalltest_include = [] +virfirewalltest_link_with = [] +if conf.has('WITH_NETWORK') + virfirewalltest_include += [ network_inc_dir ] + virfirewalltest_link_with += [ network_driver_impl ] +endif + tests += [ { 'name': 'commandtest' }, { 'name': 'cputest', 'link_with': cputest_link_with, 'link_whole': cputest_link_whole }, @@ -286,7 +293,7 @@ tests += [ { 'name': 'virerrortest' }, { 'name': 'virfilecachetest' }, { 'name': 'virfiletest' }, - { 'name': 'virfirewalltest' }, + { 'name': 'virfirewalltest', 'include': virfirewalltest_include, 'link_with': virfirewalltest_link_with }, { 'name': 'virhostcputest', 'link_whole': [ test_file_wrapper_lib ] }, { 'name': 'virhostdevtest' }, { 'name': 'viridentitytest' }, diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index 38726dcc7a..942eba90d7 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -26,6 +26,9 @@ # include "virbuffer.h" # include "virfirewall.h" +# if WITH_NETWORK +# include "network_iptables.h" +# endif # define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW # include "vircommandpriv.h" @@ -763,6 +766,113 @@ testFirewallQuery(const void *opaque G_GNUC_UNUSED) } +static void G_GNUC_UNUSED +testIPtablesSetupPrivateChainsHook(const char *const *args, + const char *const *env G_GNUC_UNUSED, + const char *input G_GNUC_UNUSED, + char **output, + char **error, + int *status, + void *opaque G_GNUC_UNUSED) +{ + if (STREQ_NULLABLE(*args, "iptables") && + STREQ_NULLABLE(*(args + 1), "-w") && + STREQ_NULLABLE(*(args + 2), "--table") && + STREQ_NULLABLE(*(args + 3), "filter") && + STREQ_NULLABLE(*(args + 4), "--list-rules")) { + *output = g_strdup("-P INPUT ACCEPT\n" + "-P FORWARD ACCEPT\n" + "-P OUTPUT ACCEPT\n" + ); + *error = NULL; + *status = EXIT_SUCCESS; + return; + } + + if (STREQ_NULLABLE(*args, "iptables") && + STREQ_NULLABLE(*(args + 1), "-w") && + STREQ_NULLABLE(*(args + 2), "--table") && + STREQ_NULLABLE(*(args + 3), "nat") && + STREQ_NULLABLE(*(args + 4), "--list-rules")) { + *output = g_strdup("-P PREROUTING ACCEPT\n" + "-P INPUT ACCEPT\n" + "-P OUTPUT ACCEPT\n" + "-P POSTROUTING ACCEPT\n"); + *error = NULL; + *status = EXIT_SUCCESS; + return; + } + + /* Intentionally steering away from empty rules above. This is how the + * table looks AFTER we've injected our rules. The idea is to cover more + * lines, esp. in iptablesPrivateChainCreate(). */ + if (STREQ_NULLABLE(*args, "iptables") && + STREQ_NULLABLE(*(args + 1), "-w") && + STREQ_NULLABLE(*(args + 2), "--table") && + STREQ_NULLABLE(*(args + 3), "mangle") && + STREQ_NULLABLE(*(args + 4), "--list-rules")) { + *output = g_strdup("-P PREROUTING ACCEPT\n" + "-P INPUT ACCEPT\n" + "-P FORWARD ACCEPT\n" + "-P OUTPUT ACCEPT\n" + "-P POSTROUTING ACCEPT\n" + "-N LIBVIRT_PRT\n" + "-A POSTROUTING -j LIBVIRT_PRT\n" + "-A LIBVIRT_PRT -o virbr0 -p udp -m udp --dport 68 -j CHECKSUM --checksum-fill\n"); + *error = NULL; + *status = EXIT_SUCCESS; + return; + } + + *output = NULL; + *error = NULL; + *status = EXIT_SUCCESS; +} + + +static int +testIPtablesSetupPrivateChains(const void *opaque G_GNUC_UNUSED) +{ +# if WITH_NETWORK + g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; + g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); + const char *actual; + const char *expected = + IPTABLES " -w --table filter --list-rules\n" + IPTABLES " -w --table nat --list-rules\n" + IPTABLES " -w --table mangle --list-rules\n" + IPTABLES " -w --table filter --new-chain LIBVIRT_INP\n" + IPTABLES " -w --table filter --insert INPUT --jump LIBVIRT_INP\n" + IPTABLES " -w --table filter --new-chain LIBVIRT_OUT\n" + IPTABLES " -w --table filter --insert OUTPUT --jump LIBVIRT_OUT\n" + IPTABLES " -w --table filter --new-chain LIBVIRT_FWO\n" + IPTABLES " -w --table filter --insert FORWARD --jump LIBVIRT_FWO\n" + IPTABLES " -w --table filter --new-chain LIBVIRT_FWI\n" + IPTABLES " -w --table filter --insert FORWARD --jump LIBVIRT_FWI\n" + IPTABLES " -w --table filter --new-chain LIBVIRT_FWX\n" + IPTABLES " -w --table filter --insert FORWARD --jump LIBVIRT_FWX\n" + IPTABLES " -w --table nat --new-chain LIBVIRT_PRT\n" + IPTABLES " -w --table nat --insert POSTROUTING --jump LIBVIRT_PRT\n"; + + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testIPtablesSetupPrivateChainsHook, NULL); + + if (iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV4) < 0) + return -1; + + actual = virBufferCurrentContent(&cmdbuf); + + if (virTestCompareToString(expected, actual) < 0) { + fprintf(stderr, "Unexpected command execution\n"); + return -1; + } + + return 0; +# else + return EXIT_AM_SKIP; +# endif +} + + static int mymain(void) { @@ -784,6 +894,7 @@ mymain(void) RUN_TEST("many rollback", testFirewallManyRollback); RUN_TEST("chained rollback", testFirewallChainedRollback); RUN_TEST("query transaction", testFirewallQuery); + RUN_TEST("setup private chains", testIPtablesSetupPrivateChains); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.51.2
On a Thursday in 2025, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
When the network driver starts up it may inject some firewall rules (e.g. for a network with NAT). So far, this scenario wasn't covered in our test suite. The reason for adding this test is twofold: the fist, check we add correct rules, the second is to
*first Jano
cover iptablesPrivateChainCreate() as its implementation is soon to be changed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> The aim of virSkipSpacesBackwards() is find the first space trailing character in given string, For instance, if the input is "Something whitespacey ", then the output should be pointing to the very first space after "y". Problem here is that the input string is constant, but the returned pointer is non-constant. This is confusing, a caller shouldn't be able to modify the string, since the input was a constant string. Therefore, make the function return a const pointer too. Under the hood the function used virTrimSpaces() which under some circumstances could modify the input string. A trick was used to hide this fact away, but to be double sure rewrite the function's body. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virstring.c | 20 +++++++++++++------- src/util/virstring.h | 2 +- src/util/virsysinfo.c | 14 +++++++------- tests/virstringtest.c | 2 +- 4 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index 7249ab4e7c..e001d76bf1 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -484,17 +484,23 @@ virTrimSpaces(char *str, char **endp) * but spaces. */ void -virSkipSpacesBackwards(const char *str, char **endp) +virSkipSpacesBackwards(const char *str, + const char **endp) { - /* Casting away const is safe, since virTrimSpaces does not - * modify string with this particular usage. */ - char *s = (char*) str; + const char *end; if (!*endp) - *endp = s + strlen(s); - virTrimSpaces(s, endp); - if (s == *endp) + end = str + strlen(str); + else + end = *endp; + + while (end > str && g_ascii_isspace(end[-1])) + end--; + + if (str == end) *endp = NULL; + else + *endp = end; } /** diff --git a/src/util/virstring.h b/src/util/virstring.h index 3d880faf0c..8c2208ece8 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -75,7 +75,7 @@ void virSkipSpaces(const char **str) ATTRIBUTE_NONNULL(1); void virSkipSpacesAndBackslash(const char **str) ATTRIBUTE_NONNULL(1); void virSkipToDigit(const char **str) ATTRIBUTE_NONNULL(1); void virTrimSpaces(char *str, char **endp) ATTRIBUTE_NONNULL(1); -void virSkipSpacesBackwards(const char *str, char **endp) +void virSkipSpacesBackwards(const char *str, const char **endp) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); bool virStringIsEmpty(const char *str); diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 0f12a8964f..c017ad34b7 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -625,7 +625,7 @@ virSysinfoParseBIOS(const char *base, virSysinfoBIOSDef **bios) { int ret = -1; const char *cur; - char *eol = NULL; + const char *eol = NULL; virSysinfoBIOSDef *def; if ((cur = strstr(base, "BIOS Information")) == NULL) @@ -679,7 +679,7 @@ virSysinfoParseX86System(const char *base, virSysinfoSystemDef **sysdef) { int ret = -1; const char *cur; - char *eol = NULL; + const char *eol = NULL; virSysinfoSystemDef *def; if ((cur = strstr(base, "System Information")) == NULL) @@ -755,7 +755,7 @@ virSysinfoParseX86BaseBoard(const char *base, size_t *nbaseBoard) { const char *cur; - char *eol = NULL; + const char *eol = NULL; virSysinfoBaseBoardDef *boards = NULL; size_t nboards = 0; @@ -832,7 +832,7 @@ virSysinfoParseX86Chassis(const char *base, { int ret = -1; const char *cur; - char *eol = NULL; + const char *eol = NULL; virSysinfoChassisDef *def; if ((cur = strstr(base, "Chassis Information")) == NULL) @@ -942,7 +942,7 @@ virSysinfoParseOEMStrings(const char *base, while ((cur = strstr(cur, "String "))) { char *collon = NULL; unsigned int idx = 0; - char *eol; + const char *eol; cur += 7; @@ -1005,7 +1005,7 @@ static void virSysinfoParseX86Processor(const char *base, virSysinfoDef *ret) { const char *cur, *tmp_base; - char *eol; + const char *eol; virSysinfoProcessorDef *processor; while ((tmp_base = strstr(base, "Processor Information")) != NULL) { @@ -1103,7 +1103,7 @@ static void virSysinfoParseX86Memory(const char *base, virSysinfoDef *ret) { const char *cur, *tmp_base; - char *eol; + const char *eol; virSysinfoMemoryDef *memory; while ((tmp_base = strstr(base, "Memory Device")) != NULL) { diff --git a/tests/virstringtest.c b/tests/virstringtest.c index a9c8e621ce..0792155cc3 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -118,7 +118,7 @@ static int testSkipSpacesBackwards(const void *opaque G_GNUC_UNUSED) { const char *str = TEST_STR TEST_SPACES; - char *eol = NULL; + const char *eol = NULL; virSkipSpacesBackwards(str, &eol); -- 2.51.2
From: Michal Privoznik <mprivozn@redhat.com> The iptablesPrivateChainCreate() function gets a NULL terminated array of strings (@lines argument), each item representing one line of iptables output. Currently, the variable used to iterate over the array is named 'tmp' which is not very descriptive. Rename it to 'line'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/network_iptables.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/network/network_iptables.c b/src/network/network_iptables.c index e8da15426e..be91b273ed 100644 --- a/src/network/network_iptables.c +++ b/src/network/network_iptables.c @@ -85,26 +85,27 @@ iptablesPrivateChainCreate(virFirewall *fw, iptablesGlobalChainData *data = opaque; g_autoptr(GHashTable) chains = virHashNew(NULL); g_autoptr(GHashTable) links = virHashNew(NULL); - const char *const *tmp; + const char *const *line; size_t i; - tmp = lines; - while (tmp && *tmp) { - if (STRPREFIX(*tmp, "-N ")) { /* eg "-N LIBVIRT_INP" */ - if (virHashUpdateEntry(chains, *tmp + 3, (void *)0x1) < 0) + line = lines; + while (line && *line) { + if (STRPREFIX(*line, "-N ")) { /* eg "-N LIBVIRT_INP" */ + if (virHashUpdateEntry(chains, *line + 3, (void *)0x1) < 0) return -1; - } else if (STRPREFIX(*tmp, "-A ")) { /* eg "-A INPUT -j LIBVIRT_INP" */ - char *sep = strchr(*tmp + 3, ' '); + } else if (STRPREFIX(*line, "-A ")) { /* eg "-A INPUT -j LIBVIRT_INP" */ + char *sep = strchr(*line + 3, ' '); + if (sep) { *sep = '\0'; if (STRPREFIX(sep + 1, "-j ")) { if (virHashUpdateEntry(links, sep + 4, - (char *)*tmp + 3) < 0) + (char *)*line + 3) < 0) return -1; } } } - tmp++; + line++; } for (i = 0; i < data->nchains; i++) { -- 2.51.2
From: Michal Privoznik <mprivozn@redhat.com> The body of iptablesPrivateChainCreate() uses STRPREFIX() to match strings starting with certain prefix. Then it uses pointer arithmetic to skip the prefix. Well, that's exactly what STRSKIP() is meant to do. Switch the body to use the latter. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/network_iptables.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/network/network_iptables.c b/src/network/network_iptables.c index be91b273ed..19dcfc7c8b 100644 --- a/src/network/network_iptables.c +++ b/src/network/network_iptables.c @@ -90,17 +90,21 @@ iptablesPrivateChainCreate(virFirewall *fw, line = lines; while (line && *line) { - if (STRPREFIX(*line, "-N ")) { /* eg "-N LIBVIRT_INP" */ - if (virHashUpdateEntry(chains, *line + 3, (void *)0x1) < 0) + const char *tmp; + + if ((tmp = STRSKIP(*line, "-N "))) { /* eg "-N LIBVIRT_INP" */ + if (virHashUpdateEntry(chains, tmp, (void *)0x1) < 0) return -1; - } else if (STRPREFIX(*line, "-A ")) { /* eg "-A INPUT -j LIBVIRT_INP" */ - char *sep = strchr(*line + 3, ' '); + } else if ((tmp = STRSKIP(*line, "-A "))) { /* eg "-A INPUT -j LIBVIRT_INP" */ + char *sep = strchr(tmp, ' '); if (sep) { + char *target; + *sep = '\0'; - if (STRPREFIX(sep + 1, "-j ")) { - if (virHashUpdateEntry(links, sep + 4, - (char *)*line + 3) < 0) + if ((target = STRSKIP(sep + 1, "-j "))) { + if (virHashUpdateEntry(links, target, + (char *)tmp) < 0) return -1; } } -- 2.51.2
From: Michal Privoznik <mprivozn@redhat.com> The iptablesPrivateChainCreate() function is given an array of const strings. This constitutes a promise to the caller that the data is not modified. But inside the data is modified anyway (to cut out some parts of the data). Well, with a help from g_strdup() the promise can be kept. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/network_iptables.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/network/network_iptables.c b/src/network/network_iptables.c index 19dcfc7c8b..d21ce59b70 100644 --- a/src/network/network_iptables.c +++ b/src/network/network_iptables.c @@ -84,7 +84,7 @@ iptablesPrivateChainCreate(virFirewall *fw, { iptablesGlobalChainData *data = opaque; g_autoptr(GHashTable) chains = virHashNew(NULL); - g_autoptr(GHashTable) links = virHashNew(NULL); + g_autoptr(GHashTable) links = virHashNew(g_free); const char *const *line; size_t i; @@ -96,16 +96,18 @@ iptablesPrivateChainCreate(virFirewall *fw, if (virHashUpdateEntry(chains, tmp, (void *)0x1) < 0) return -1; } else if ((tmp = STRSKIP(*line, "-A "))) { /* eg "-A INPUT -j LIBVIRT_INP" */ - char *sep = strchr(tmp, ' '); + const char *sep = strchr(tmp, ' '); if (sep) { - char *target; + const char *target; - *sep = '\0'; if ((target = STRSKIP(sep + 1, "-j "))) { - if (virHashUpdateEntry(links, target, - (char *)tmp) < 0) + char *chain = g_strndup(tmp, sep - tmp); + + if (virHashUpdateEntry(links, target, chain) < 0) { + g_free(chain); return -1; + } } } } -- 2.51.2
From: Michal Privoznik <mprivozn@redhat.com> There's new commit in glibc [1] which makes memchr(), strchr(), strrchr(), strpbrk() and strstr() reflect type of the input string. If it's a constant string, then the return type of these functions is also 'const char *'. But this change tickles -Wincompatible-pointer-types-discards-qualifiers warning. And indeed, there are some places where we use a 'char *' typed variable to store the retval, or even misuse the fact 'char *' is returned and modify const string. To fix this, a couple of different approaches is used: a) switch variable type to 'const char *', b) switch argument to 'char *' (in a few places we have strdup()-ed) the const string already, c) strdup() the string and use b). 1: https://sourceware.org/git/?p=glibc.git;a=commit;h=cd748a63ab1a7ae846175c532... Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/interface/interface_backend_udev.c | 2 +- src/libxl/xen_common.c | 10 +++++----- src/libxl/xen_xm.c | 10 ++++++---- src/node_device/node_device_udev.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 4 ++-- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_nbdkit.c | 2 +- src/rpc/virnetsshsession.c | 5 +++-- src/storage/storage_util.c | 2 +- src/storage_file/storage_source.c | 4 ++-- src/util/vircgroup.c | 2 +- src/util/virfile.c | 2 +- src/util/virstoragefile.c | 4 ++-- src/util/virsysinfo.c | 8 ++++---- src/util/virxml.c | 3 ++- src/vmware/vmware_conf.c | 2 +- tools/vsh.c | 6 +++--- 17 files changed, 37 insertions(+), 33 deletions(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 48eacdcdc2..c6d14aa803 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -927,7 +927,7 @@ udevGetIfaceDef(struct udev *udev, const char *name) g_autoptr(virInterfaceDef) ifacedef = NULL; unsigned int mtu; const char *mtu_str; - char *vlan_parent_dev = NULL; + const char *vlan_parent_dev = NULL; const char *devtype; /* Allocate our interface definition structure */ diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 666c6cae20..890ef11723 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -802,20 +802,20 @@ static virDomainChrDef * xenParseSxprChar(const char *value, const char *tty) { - const char *prefix; + g_autofree char *prefix = NULL; char *tmp; virDomainChrDef *def; if (!(def = virDomainChrDefNew(NULL))) return NULL; - prefix = value; + prefix = g_strdup(value); if (g_path_is_absolute(value)) { def->source->type = VIR_DOMAIN_CHR_TYPE_DEV; def->source->data.file.path = g_strdup(value); } else { - if ((tmp = strchr(value, ':')) != NULL) { + if ((tmp = strchr(prefix, ':')) != NULL) { *tmp = '\0'; value = tmp + 1; } @@ -1019,7 +1019,7 @@ xenParseCharDev(virConf *conf, virDomainDef *def, const char *nativeFormat) static int xenParseVifBridge(virDomainNetDef *net, const char *bridge) { - char *vlanstr; + const char *vlanstr; unsigned int tag; if ((vlanstr = strchr(bridge, '.'))) { @@ -1144,7 +1144,7 @@ xenParseVif(char *entry, const char *vif_typename) for (keyval = keyvals; keyval && *keyval; keyval++) { const char *key = *keyval; - char *val = strchr(key, '='); + const char *val = strchr(key, '='); virSkipSpaces(&key); diff --git a/src/libxl/xen_xm.c b/src/libxl/xen_xm.c index 274b35153b..4327a9391d 100644 --- a/src/libxl/xen_xm.c +++ b/src/libxl/xen_xm.c @@ -154,9 +154,11 @@ xenParseXMDisk(char *entry, int hvm) src = virDomainDiskGetSource(disk); if (src) { size_t len; + const char *sep; + /* The main type phy:, file:, tap: ... */ - if ((tmp = strchr(src, ':')) != NULL) { - len = tmp - src; + if ((sep = strchr(src, ':')) != NULL) { + len = sep - src; tmp = g_strndup(src, len); virDomainDiskSetDriver(disk, tmp); @@ -173,9 +175,9 @@ xenParseXMDisk(char *entry, int hvm) STREQ_NULLABLE(virDomainDiskGetDriver(disk), "tap2")) { char *driverType; - if (!(tmp = strchr(src, ':'))) + if (!(sep = strchr(src, ':'))) goto error; - len = tmp - src; + len = sep - src; driverType = g_strndup(src, len); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 27e62febe8..20a525bcec 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1187,7 +1187,7 @@ static int udevGetCCWAddress(const char *sysfs_path, virNodeDevCapData *data) { - char *p; + const char *p; g_autofree virCCWDeviceAddress *ccw_addr = g_new0(virCCWDeviceAddress, 1); if ((p = strrchr(sysfs_path, '/')) == NULL || diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 4578152670..859347409c 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2604,7 +2604,7 @@ ebtablesRemoveSubChainsQuery(virFirewall *fw, const char *chainprefixes = opaque; for (i = 0; lines[i] != NULL; i++) { - char *tmp = strstr(lines[i], "-j "); + const char *tmp = strstr(lines[i], "-j "); VIR_DEBUG("Considering '%s'", lines[i]); @@ -2708,7 +2708,7 @@ ebtablesRenameTmpSubAndRootChainsQuery(virFirewall *fw, char newchain[MAX_CHAINNAME_LENGTH]; for (i = 0; lines[i] != NULL; i++) { - char *tmp = strstr(lines[i], "-j "); + const char *tmp = strstr(lines[i], "-j "); VIR_DEBUG("Considering '%s'", lines[i]); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 494d7ef515..a602b1e65b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -241,7 +241,7 @@ qemuMonitorJSONIOProcess(qemuMonitor *mon, /*VIR_DEBUG("Data %d bytes [%s]", len, data);*/ while (used < len) { - char *nl = strstr(data + used, LINE_ENDING); + const char *nl = strstr(data + used, LINE_ENDING); if (nl) { int got = nl - (data + used); diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index c1bc6bc363..542a6b1f44 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -146,7 +146,7 @@ qemuNbdkitCapsQueryBuildConfig(qemuNbdkitCaps *nbdkit) size_t i; g_autofree char *output = NULL; g_auto(GStrv) lines = NULL; - const char *line; + char *line; g_autoptr(virCommand) cmd = virCommandNewArgList(nbdkit->path, "--dump-config", NULL); diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 316521d4cf..83c8630480 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -216,13 +216,14 @@ virNetSSHKbIntCb(const char *name G_GNUC_UNUSED, /* fill data structures for auth callback */ for (i = 0; i < num_prompts; i++) { - askcred[i].prompt = g_strdup((char*)prompts[i].text); + char *prompt = g_strdup((char*)prompts[i].text); /* remove colon and trailing spaces from prompts, as default behavior * of libvirt's auth callback is to add them */ - if ((tmp = strrchr(askcred[i].prompt, ':'))) + if ((tmp = strrchr(prompt, ':'))) *tmp = '\0'; + askcred[i].prompt = prompt; askcred[i].type = prompts[i].echo ? credtype_echo : credtype_noecho; } diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 8f7fc6cc01..e2b41de1f2 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3773,7 +3773,7 @@ getOldStyleBlockDevice(const char *lun_path G_GNUC_UNUSED, const char *block_name, char **block_device) { - char *blockp = NULL; + const char *blockp = NULL; /* old-style; just parse out the sd */ if (!(blockp = strrchr(block_name, ':'))) { diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index 843910a0d8..1883225a8b 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -45,8 +45,8 @@ VIR_LOG_INIT("storage_source"); static bool virStorageSourceBackinStoreStringIsFile(const char *backing) { - char *colon; - char *slash; + const char *colon; + const char *slash; if (!backing) return false; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 532a7e5690..3d66d3acb2 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -517,7 +517,7 @@ int virCgroupSetValueRaw(const char *path, const char *value) { - char *tmp; + const char *tmp; VIR_DEBUG("Set path '%s' to value '%s'", path, value); if (virFileWriteStr(path, value, 0) < 0) { diff --git a/src/util/virfile.c b/src/util/virfile.c index a5c9fbe0d9..f195d02e29 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3359,7 +3359,7 @@ char * virFileSanitizePath(const char *path) { const char *cur = path; - char *uri; + const char *uri; char *cleanpath; int idx = 0; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 031baa1407..22bda87643 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -119,8 +119,8 @@ virStorageFileGetNPIVKey(const char *path, char **key) { int status; - const char *serial; - const char *port; + char *serial; + char *port; g_autofree char *outbuf = NULL; g_autoptr(virCommand) cmd = NULL; diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index c017ad34b7..b638fbd16c 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -219,7 +219,7 @@ static int virSysinfoParsePPCSystem(const char *base, virSysinfoSystemDef **sysdef) { int ret = -1; - char *eol = NULL; + const char *eol = NULL; const char *cur; virSysinfoSystemDef *def; @@ -267,7 +267,7 @@ static void virSysinfoParsePPCProcessor(const char *base, virSysinfoDef *ret) { const char *cur; - char *eol, *tmp_base; + const char *eol, *tmp_base; virSysinfoProcessorDef *processor; while ((tmp_base = strstr(base, "processor")) != NULL) { @@ -336,7 +336,7 @@ static int virSysinfoParseARMSystem(const char *base, virSysinfoSystemDef **sysdef) { int ret = -1; - char *eol = NULL; + const char *eol = NULL; const char *cur; virSysinfoSystemDef *def; @@ -384,7 +384,7 @@ static void virSysinfoParseARMProcessor(const char *base, virSysinfoDef *ret) { const char *cur; - char *eol, *tmp_base; + const char *eol, *tmp_base; virSysinfoProcessorDef *processor; char *processor_type = NULL; diff --git a/src/util/virxml.c b/src/util/virxml.c index 77c7b5a8f4..274f072598 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -368,7 +368,8 @@ virXMLCheckIllegalChars(const char *nodeName, const char *str, const char *illegal) { - char *c; + const char *c; + if ((c = strpbrk(str, illegal))) { virReportError(VIR_ERR_XML_DETAIL, _("invalid char in %1$s: %2$c"), nodeName, *c); diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index a598b512dc..11db044b52 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -305,7 +305,7 @@ vmwareDomainConfigDisplay(vmwareDomainPtr pDomain, virDomainDef *def) static int vmwareParsePath(const char *path, char **directory, char **filename) { - char *separator; + const char *separator; separator = strrchr(path, '/'); diff --git a/tools/vsh.c b/tools/vsh.c index 4aacc5feac..69d3930e43 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -413,7 +413,7 @@ vshCmddefCheckInternals(vshControl *ctl, case VSH_OT_ALIAS: { size_t j; g_autofree char *name = NULL; - char *p; + const char *p; if (opt->required || opt->positional || @@ -502,7 +502,7 @@ vshCmdGetOption(vshControl *ctl, alias of option and its default value */ alias = g_strdup(n->def->help); name = alias; - if ((value = strchr(name, '='))) { + if ((value = strchr(alias, '='))) { *value = '\0'; if (*optstr) { if (report) @@ -1660,7 +1660,7 @@ vshCommandParse(vshControl *ctl, * value * -- (terminate accepting '--option', fill only positional args) */ - const char *optionname = tkdata + 2; + char *optionname = tkdata + 2; char *sep; if (!STRPREFIX(tkdata, "--")) { -- 2.51.2
On Thu, Nov 27, 2025 at 02:50:55PM +0100, Michal Privoznik via Devel wrote:
This stems from the discussion here:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HVMCJ...
Green pipeline here:
https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/2183169334
Michal Prívozník (8): vircommand: Update documentation to virCommandSetDryRun() virstringtest: Introduce a test for virSkipSpacesBackwards() virfirewalltest: Introduce testIPtablesSetupPrivateChains() virSkipSpacesBackwards: Turn @endp into const iptablesPrivateChainCreate: Rename @tmp variable iptablesPrivateChainCreate: Switch to STRSKIP() iptablesPrivateChainCreate: Avoid modifying const string lib: Avoid changing const strings via strchr() and friends
src/interface/interface_backend_udev.c | 2 +- src/libxl/xen_common.c | 10 +- src/libxl/xen_xm.c | 10 +- src/network/meson.build | 1 + src/network/network_iptables.c | 33 ++++--- src/node_device/node_device_udev.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 4 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_nbdkit.c | 2 +- src/rpc/virnetsshsession.c | 5 +- src/storage/storage_util.c | 2 +- src/storage_file/storage_source.c | 4 +- src/util/vircgroup.c | 2 +- src/util/vircommand.c | 10 +- src/util/virfile.c | 2 +- src/util/virstoragefile.c | 4 +- src/util/virstring.c | 20 ++-- src/util/virstring.h | 2 +- src/util/virsysinfo.c | 22 ++--- src/util/virxml.c | 3 +- src/vmware/vmware_conf.c | 2 +- tests/meson.build | 9 +- tests/virfirewalltest.c | 111 ++++++++++++++++++++++ tests/virstringtest.c | 48 ++++++++++ tools/vsh.c | 6 +- 25 files changed, 252 insertions(+), 66 deletions(-)
-- 2.51.2
Tested on macOS 26.1. Tested-by: Jaroslav Suchanek <jsuchane@redhat.com>
On a Thursday in 2025, Michal Privoznik via Devel wrote:
This stems from the discussion here:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HVMCJ...
Green pipeline here:
https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/2183169334
Michal Prívozník (8): vircommand: Update documentation to virCommandSetDryRun() virstringtest: Introduce a test for virSkipSpacesBackwards() virfirewalltest: Introduce testIPtablesSetupPrivateChains() virSkipSpacesBackwards: Turn @endp into const iptablesPrivateChainCreate: Rename @tmp variable iptablesPrivateChainCreate: Switch to STRSKIP() iptablesPrivateChainCreate: Avoid modifying const string lib: Avoid changing const strings via strchr() and friends
src/interface/interface_backend_udev.c | 2 +- src/libxl/xen_common.c | 10 +- src/libxl/xen_xm.c | 10 +- src/network/meson.build | 1 + src/network/network_iptables.c | 33 ++++--- src/node_device/node_device_udev.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 4 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_nbdkit.c | 2 +- src/rpc/virnetsshsession.c | 5 +- src/storage/storage_util.c | 2 +- src/storage_file/storage_source.c | 4 +- src/util/vircgroup.c | 2 +- src/util/vircommand.c | 10 +- src/util/virfile.c | 2 +- src/util/virstoragefile.c | 4 +- src/util/virstring.c | 20 ++-- src/util/virstring.h | 2 +- src/util/virsysinfo.c | 22 ++--- src/util/virxml.c | 3 +- src/vmware/vmware_conf.c | 2 +- tests/meson.build | 9 +- tests/virfirewalltest.c | 111 ++++++++++++++++++++++ tests/virstringtest.c | 48 ++++++++++ tools/vsh.c | 6 +- 25 files changed, 252 insertions(+), 66 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Jaroslav Suchanek -
Ján Tomko -
Michal Privoznik