All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] util/envlist: fix prefix-match in name lookup
@ 2026-05-20 21:26 Denis V. Lunev via qemu development
  2026-05-20 21:26 ` [PATCH 1/2] util/envlist: fix prefix-match in envlist_unsetenv() " Denis V. Lunev via qemu development
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Denis V. Lunev via qemu development @ 2026-05-20 21:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, Stefan Hajnoczi, Markus Armbruster, Paolo Bonzini

A bug report against our downstream tree turned out to have its root
cause in plain mainstream code: envlist_unsetenv() does a prefix-match
lookup that drops the wrong entry when one stored name happens to be a
prefix of another. The downstream symptom is specific to our setup and
isn't interesting here -- the underlying lookup mistake is the part
worth fixing, and it is reachable from a normal qemu-user invocation
through the -U command-line option, so the fix belongs upstream.

Patch 1 fixes the lookup: each entry now stores its name length at
insertion time, and a tiny helper compares with explicit length
equality plus memcmp. envlist_setenv()'s self-search was accidentally
safe (it included the '=' byte in its strncmp window and that '='
served as a boundary) but is converted to the same helper so the name
boundary becomes a structural property of the entry rather than a
property of its byte layout. Without that, the two sites can easily
drift apart again.

Patch 2 backfils test coverage for util/envlist -- there was none --
in tests/unit/test-envlist. I verified that the regression case
(envlist_unsetenv("FOO") vs. a stored "FOOBAR=...") fails against the
pre-fix code and passes after the fix.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Denis V. Lunev (2):
  util/envlist: fix prefix-match in envlist_unsetenv() name lookup
  tests/unit: add test-envlist covering setenv/unsetenv name matching

 tests/unit/meson.build    |   1 +
 tests/unit/test-envlist.c | 196 ++++++++++++++++++++++++++++++++++++++
 util/envlist.c            |  19 +++-
 3 files changed, 212 insertions(+), 4 deletions(-)
 create mode 100644 tests/unit/test-envlist.c

-- 
2.51.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] util/envlist: fix prefix-match in envlist_unsetenv() name lookup
  2026-05-20 21:26 [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev via qemu development
@ 2026-05-20 21:26 ` Denis V. Lunev via qemu development
  2026-05-20 21:26 ` [PATCH 2/2] tests/unit: add test-envlist covering setenv/unsetenv name matching Denis V. Lunev via qemu development
  2026-05-28  9:24 ` [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev
  2 siblings, 0 replies; 7+ messages in thread
From: Denis V. Lunev via qemu development @ 2026-05-20 21:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, Stefan Hajnoczi, Markus Armbruster, Paolo Bonzini

envlist_unsetenv() looked up the entry to remove with
strncmp(entry->ev_var, env, strlen(env)). The comparison length is
the requested name's length, so any stored entry whose name *starts*
with that name compares equal. envlist_setenv() inserts at the head
of the list, so the first hit wins: with FOO=... stored first and
FOOBAR=... stored afterward, envlist_unsetenv("FOO") iterates from
the head, matches FOOBAR=... on the prefix, and drops it instead of
FOO=...

linux-user and bsd-user reach this code via the -U command-line
switch, so the bug is reachable from a normal qemu-user invocation.

envlist_setenv() used the same strncmp pattern but with
envname_len = (eq_sign - env + 1), so the '=' byte sat inside the
compared window and acted as an implicit boundary. setenv was
therefore not buggy -- but the safety lived in the byte layout of
ev_var rather than in the entry, so a future edit could easily
drift the two sites apart again.

Store the name length on each entry at insertion time and compare
with explicit length equality plus memcmp via a small helper. Use
the helper at both lookup sites so the boundary becomes a
structural property of the entry: envlist_unsetenv() stops
prefix-matching, and envlist_setenv()'s self-search no longer
depends on the '=' byte serving as a sentinel.

Fixes: 04a6dfebb6b5 ("linux-user: Add generic env variable handling")
Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 util/envlist.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/util/envlist.c b/util/envlist.c
index 15fdbb109d..196c92c190 100644
--- a/util/envlist.c
+++ b/util/envlist.c
@@ -3,7 +3,8 @@
 #include "qemu/envlist.h"
 
 struct envlist_entry {
-    const char *ev_var;            /* actual env value */
+    const char *ev_var;            /* actual env value: "NAME=VALUE" */
+    size_t ev_name_len;            /* length of NAME (offset of '=') */
     QLIST_ENTRY(envlist_entry) ev_link;
 };
 
@@ -12,6 +13,13 @@ struct envlist {
     size_t el_count;                        /* number of entries */
 };
 
+static inline bool envlist_name_eq(const struct envlist_entry *entry,
+                                   const char *name, size_t name_len)
+{
+    return entry->ev_name_len == name_len &&
+           memcmp(entry->ev_var, name, name_len) == 0;
+}
+
 /*
  * Allocates new envlist and returns pointer to it.
  */
@@ -67,7 +75,7 @@ envlist_setenv(envlist_t *envlist, const char *env)
     /* find out first equals sign in given env */
     if ((eq_sign = strchr(env, '=')) == NULL)
         return (EINVAL);
-    envname_len = eq_sign - env + 1;
+    envname_len = eq_sign - env;
 
     /*
      * If there already exists variable with given name
@@ -76,8 +84,9 @@ envlist_setenv(envlist_t *envlist, const char *env)
      */
     for (entry = envlist->el_entries.lh_first; entry != NULL;
         entry = entry->ev_link.le_next) {
-        if (strncmp(entry->ev_var, env, envname_len) == 0)
+        if (envlist_name_eq(entry, env, envname_len)) {
             break;
+        }
     }
 
     if (entry != NULL) {
@@ -90,6 +99,7 @@ envlist_setenv(envlist_t *envlist, const char *env)
 
     entry = g_malloc(sizeof(*entry));
     entry->ev_var = g_strdup(env);
+    entry->ev_name_len = envname_len;
     QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link);
 
     return (0);
@@ -119,8 +129,9 @@ envlist_unsetenv(envlist_t *envlist, const char *env)
     envname_len = strlen(env);
     for (entry = envlist->el_entries.lh_first; entry != NULL;
         entry = entry->ev_link.le_next) {
-        if (strncmp(entry->ev_var, env, envname_len) == 0)
+        if (envlist_name_eq(entry, env, envname_len)) {
             break;
+        }
     }
     if (entry != NULL) {
         QLIST_REMOVE(entry, ev_link);
-- 
2.51.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] tests/unit: add test-envlist covering setenv/unsetenv name matching
  2026-05-20 21:26 [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev via qemu development
  2026-05-20 21:26 ` [PATCH 1/2] util/envlist: fix prefix-match in envlist_unsetenv() " Denis V. Lunev via qemu development
@ 2026-05-20 21:26 ` Denis V. Lunev via qemu development
  2026-05-28  9:24 ` [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev
  2 siblings, 0 replies; 7+ messages in thread
From: Denis V. Lunev via qemu development @ 2026-05-20 21:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, Stefan Hajnoczi, Markus Armbruster, Paolo Bonzini

util/envlist had no test coverage. Add tests/unit/test-envlist
exercising the public envlist API and pinning down the prefix-match
hazard fixed in the previous commit:

  - envlist_unsetenv("FOO") must not remove an entry named "FOOBAR";
  - envlist_setenv("FOO=...") must not replace an existing "FOOBAR=..."
    entry placed earlier in the list (envlist_setenv() inserts at the
    head, so the first prefix match wins under the old strncmp rule).

Also cover the rest of the contract: head-insertion order observed
through envlist_to_environ(), replacement of an existing variable,
the count argument of envlist_to_environ(), and the documented EINVAL
paths (NULL inputs, setenv without '=', unsetenv with '=').

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/unit/meson.build    |   1 +
 tests/unit/test-envlist.c | 196 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 197 insertions(+)
 create mode 100644 tests/unit/test-envlist.c

diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index de64f9501f..01cc540a45 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -49,6 +49,7 @@ tests = {
   'test-qapi-util': [],
   'test-interval-tree': [],
   'test-fifo': [],
+  'test-envlist': [],
 }
 
 if have_system or have_tools
diff --git a/tests/unit/test-envlist.c b/tests/unit/test-envlist.c
new file mode 100644
index 0000000000..53813dd4de
--- /dev/null
+++ b/tests/unit/test-envlist.c
@@ -0,0 +1,196 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * envlist tests
+ *
+ * Copyright 2026 Virtuozzo International GmbH
+ *
+ * Authors:
+ *  Denis V. Lunev <den@openvz.org>
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/envlist.h"
+
+static void free_environ(char **env)
+{
+    char **p;
+
+    for (p = env; *p != NULL; p++) {
+        g_free(*p);
+    }
+    g_free(env);
+}
+
+static const char *find_env(char **env, const char *name)
+{
+    size_t name_len = strlen(name);
+    char **p;
+
+    for (p = env; *p != NULL; p++) {
+        if (strncmp(*p, name, name_len) == 0 && (*p)[name_len] == '=') {
+            return *p + name_len + 1;
+        }
+    }
+    return NULL;
+}
+
+static void test_envlist_basic(void)
+{
+    envlist_t *el = envlist_create();
+    char **env;
+    size_t count;
+
+    /* empty list */
+    env = envlist_to_environ(el, &count);
+    g_assert_cmpuint(count, ==, 0);
+    g_assert_null(env[0]);
+    free_environ(env);
+
+    /* add */
+    g_assert_cmpint(envlist_setenv(el, "A=1"), ==, 0);
+    g_assert_cmpint(envlist_setenv(el, "B=2"), ==, 0);
+
+    env = envlist_to_environ(el, &count);
+    g_assert_cmpuint(count, ==, 2);
+    g_assert_cmpstr(find_env(env, "A"), ==, "1");
+    g_assert_cmpstr(find_env(env, "B"), ==, "2");
+    free_environ(env);
+
+    /* replace */
+    g_assert_cmpint(envlist_setenv(el, "A=42"), ==, 0);
+    env = envlist_to_environ(el, &count);
+    g_assert_cmpuint(count, ==, 2);
+    g_assert_cmpstr(find_env(env, "A"), ==, "42");
+    g_assert_cmpstr(find_env(env, "B"), ==, "2");
+    free_environ(env);
+
+    /* unset existing */
+    g_assert_cmpint(envlist_unsetenv(el, "A"), ==, 0);
+    env = envlist_to_environ(el, &count);
+    g_assert_cmpuint(count, ==, 1);
+    g_assert_null(find_env(env, "A"));
+    g_assert_cmpstr(find_env(env, "B"), ==, "2");
+    free_environ(env);
+
+    /* unset non-existing is a no-op success */
+    g_assert_cmpint(envlist_unsetenv(el, "NOPE"), ==, 0);
+    env = envlist_to_environ(el, &count);
+    g_assert_cmpuint(count, ==, 1);
+    free_environ(env);
+
+    envlist_free(el);
+}
+
+/*
+ * envlist_setenv() inserts at the head; envlist_to_environ() walks
+ * head-to-tail, so the last setenv comes out first.
+ */
+static void test_envlist_head_insertion_order(void)
+{
+    envlist_t *el = envlist_create();
+    char **env;
+    size_t count;
+
+    g_assert_cmpint(envlist_setenv(el, "A=1"), ==, 0);
+    g_assert_cmpint(envlist_setenv(el, "B=2"), ==, 0);
+    g_assert_cmpint(envlist_setenv(el, "C=3"), ==, 0);
+
+    env = envlist_to_environ(el, &count);
+    g_assert_cmpuint(count, ==, 3);
+    g_assert_cmpstr(env[0], ==, "C=3");
+    g_assert_cmpstr(env[1], ==, "B=2");
+    g_assert_cmpstr(env[2], ==, "A=1");
+    g_assert_null(env[3]);
+
+    free_environ(env);
+    envlist_free(el);
+}
+
+static void test_envlist_einval(void)
+{
+    envlist_t *el = envlist_create();
+
+    /* NULL list */
+    g_assert_cmpint(envlist_setenv(NULL, "A=1"), ==, EINVAL);
+    g_assert_cmpint(envlist_unsetenv(NULL, "A"), ==, EINVAL);
+
+    /* NULL string */
+    g_assert_cmpint(envlist_setenv(el, NULL), ==, EINVAL);
+    g_assert_cmpint(envlist_unsetenv(el, NULL), ==, EINVAL);
+
+    /* setenv: missing '=' */
+    g_assert_cmpint(envlist_setenv(el, "NOEQ"), ==, EINVAL);
+
+    /* unsetenv: name must not contain '=' */
+    g_assert_cmpint(envlist_unsetenv(el, "A=B"), ==, EINVAL);
+
+    envlist_free(el);
+}
+
+/*
+ * Regression: envlist_unsetenv("FOO") must not remove an entry named
+ * "FOOBAR" -- the previous strncmp(entry, name, strlen(name)) lookup
+ * prefix-matched. To trigger the bug, the longer-named entry has to
+ * be ahead of the target in the list: envlist_setenv() inserts at
+ * the head, so we add FOO first and FOOBAR last.
+ */
+static void test_envlist_unsetenv_no_prefix_match(void)
+{
+    envlist_t *el = envlist_create();
+    char **env;
+    size_t count;
+
+    g_assert_cmpint(envlist_setenv(el, "FOO=y"), ==, 0);
+    g_assert_cmpint(envlist_setenv(el, "FOOBAR=x"), ==, 0);
+
+    g_assert_cmpint(envlist_unsetenv(el, "FOO"), ==, 0);
+
+    env = envlist_to_environ(el, &count);
+    g_assert_cmpuint(count, ==, 1);
+    g_assert_cmpstr(find_env(env, "FOOBAR"), ==, "x");
+    g_assert_null(find_env(env, "FOO"));
+
+    free_environ(env);
+    envlist_free(el);
+}
+
+/*
+ * envlist_setenv() must not replace a prior FOOBAR=... entry when
+ * setting FOO=... The pre-fix code happened to be safe here only
+ * because it included the trailing '=' byte in its strncmp length;
+ * this test pins down the post-fix contract that the name boundary
+ * is a property of the entry, not of the encoded form.
+ */
+static void test_envlist_setenv_no_prefix_match(void)
+{
+    envlist_t *el = envlist_create();
+    char **env;
+    size_t count;
+
+    g_assert_cmpint(envlist_setenv(el, "FOOBAR=x"), ==, 0);
+    g_assert_cmpint(envlist_setenv(el, "FOO=y"), ==, 0);
+
+    env = envlist_to_environ(el, &count);
+    g_assert_cmpuint(count, ==, 2);
+    g_assert_cmpstr(find_env(env, "FOOBAR"), ==, "x");
+    g_assert_cmpstr(find_env(env, "FOO"), ==, "y");
+
+    free_environ(env);
+    envlist_free(el);
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/envlist/basic", test_envlist_basic);
+    g_test_add_func("/envlist/head_insertion_order",
+                    test_envlist_head_insertion_order);
+    g_test_add_func("/envlist/einval", test_envlist_einval);
+    g_test_add_func("/envlist/unsetenv_no_prefix_match",
+                    test_envlist_unsetenv_no_prefix_match);
+    g_test_add_func("/envlist/setenv_no_prefix_match",
+                    test_envlist_setenv_no_prefix_match);
+
+    return g_test_run();
+}
-- 
2.51.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] util/envlist: fix prefix-match in name lookup
  2026-05-20 21:26 [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev via qemu development
  2026-05-20 21:26 ` [PATCH 1/2] util/envlist: fix prefix-match in envlist_unsetenv() " Denis V. Lunev via qemu development
  2026-05-20 21:26 ` [PATCH 2/2] tests/unit: add test-envlist covering setenv/unsetenv name matching Denis V. Lunev via qemu development
@ 2026-05-28  9:24 ` Denis V. Lunev
  2026-06-01 19:12   ` Stefan Hajnoczi
                     ` (2 more replies)
  2 siblings, 3 replies; 7+ messages in thread
From: Denis V. Lunev @ 2026-05-28  9:24 UTC (permalink / raw)
  To: Denis V. Lunev, qemu-devel
  Cc: Stefan Hajnoczi, Markus Armbruster, Paolo Bonzini

On 5/20/26 23:26, Denis V. Lunev wrote:
> A bug report against our downstream tree turned out to have its root
> cause in plain mainstream code: envlist_unsetenv() does a prefix-match
> lookup that drops the wrong entry when one stored name happens to be a
> prefix of another. The downstream symptom is specific to our setup and
> isn't interesting here -- the underlying lookup mistake is the part
> worth fixing, and it is reachable from a normal qemu-user invocation
> through the -U command-line option, so the fix belongs upstream.
>
> Patch 1 fixes the lookup: each entry now stores its name length at
> insertion time, and a tiny helper compares with explicit length
> equality plus memcmp. envlist_setenv()'s self-search was accidentally
> safe (it included the '=' byte in its strncmp window and that '='
> served as a boundary) but is converted to the same helper so the name
> boundary becomes a structural property of the entry rather than a
> property of its byte layout. Without that, the two sites can easily
> drift apart again.
>
> Patch 2 backfils test coverage for util/envlist -- there was none --
> in tests/unit/test-envlist. I verified that the regression case
> (envlist_unsetenv("FOO") vs. a stored "FOOBAR=...") fails against the
> pre-fix code and passes after the fix.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
>
> Denis V. Lunev (2):
>   util/envlist: fix prefix-match in envlist_unsetenv() name lookup
>   tests/unit: add test-envlist covering setenv/unsetenv name matching
>
>  tests/unit/meson.build    |   1 +
>  tests/unit/test-envlist.c | 196 ++++++++++++++++++++++++++++++++++++++
>  util/envlist.c            |  19 +++-
>  3 files changed, 212 insertions(+), 4 deletions(-)
>  create mode 100644 tests/unit/test-envlist.c
>
ping


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] util/envlist: fix prefix-match in name lookup
  2026-05-28  9:24 ` [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev
@ 2026-06-01 19:12   ` Stefan Hajnoczi
  2026-06-01 19:18   ` Stefan Hajnoczi
  2026-06-03 16:51   ` Stefan Hajnoczi
  2 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2026-06-01 19:12 UTC (permalink / raw)
  To: Denis V. Lunev
  Cc: Denis V. Lunev, qemu-devel, Markus Armbruster, Paolo Bonzini,
	Laurent Vivier, Helge Deller, Warner Losh

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

On Thu, May 28, 2026 at 11:24:48AM +0200, Denis V. Lunev wrote:
> On 5/20/26 23:26, Denis V. Lunev wrote:
> > A bug report against our downstream tree turned out to have its root
> > cause in plain mainstream code: envlist_unsetenv() does a prefix-match
> > lookup that drops the wrong entry when one stored name happens to be a
> > prefix of another. The downstream symptom is specific to our setup and
> > isn't interesting here -- the underlying lookup mistake is the part
> > worth fixing, and it is reachable from a normal qemu-user invocation
> > through the -U command-line option, so the fix belongs upstream.
> >
> > Patch 1 fixes the lookup: each entry now stores its name length at
> > insertion time, and a tiny helper compares with explicit length
> > equality plus memcmp. envlist_setenv()'s self-search was accidentally
> > safe (it included the '=' byte in its strncmp window and that '='
> > served as a boundary) but is converted to the same helper so the name
> > boundary becomes a structural property of the entry rather than a
> > property of its byte layout. Without that, the two sites can easily
> > drift apart again.
> >
> > Patch 2 backfils test coverage for util/envlist -- there was none --
> > in tests/unit/test-envlist. I verified that the regression case
> > (envlist_unsetenv("FOO") vs. a stored "FOOBAR=...") fails against the
> > pre-fix code and passes after the fix.
> >
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> >
> > Denis V. Lunev (2):
> >   util/envlist: fix prefix-match in envlist_unsetenv() name lookup
> >   tests/unit: add test-envlist covering setenv/unsetenv name matching
> >
> >  tests/unit/meson.build    |   1 +
> >  tests/unit/test-envlist.c | 196 ++++++++++++++++++++++++++++++++++++++
> >  util/envlist.c            |  19 +++-
> >  3 files changed, 212 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/unit/test-envlist.c

CCing Laurent, Helge, and Warner in case they are interested in this
-user fix.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] util/envlist: fix prefix-match in name lookup
  2026-05-28  9:24 ` [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev
  2026-06-01 19:12   ` Stefan Hajnoczi
@ 2026-06-01 19:18   ` Stefan Hajnoczi
  2026-06-03 16:51   ` Stefan Hajnoczi
  2 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2026-06-01 19:18 UTC (permalink / raw)
  To: Denis V. Lunev
  Cc: Denis V. Lunev, qemu-devel, Markus Armbruster, Paolo Bonzini

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

On Thu, May 28, 2026 at 11:24:48AM +0200, Denis V. Lunev wrote:
> On 5/20/26 23:26, Denis V. Lunev wrote:
> > A bug report against our downstream tree turned out to have its root
> > cause in plain mainstream code: envlist_unsetenv() does a prefix-match
> > lookup that drops the wrong entry when one stored name happens to be a
> > prefix of another. The downstream symptom is specific to our setup and
> > isn't interesting here -- the underlying lookup mistake is the part
> > worth fixing, and it is reachable from a normal qemu-user invocation
> > through the -U command-line option, so the fix belongs upstream.
> >
> > Patch 1 fixes the lookup: each entry now stores its name length at
> > insertion time, and a tiny helper compares with explicit length
> > equality plus memcmp. envlist_setenv()'s self-search was accidentally
> > safe (it included the '=' byte in its strncmp window and that '='
> > served as a boundary) but is converted to the same helper so the name
> > boundary becomes a structural property of the entry rather than a
> > property of its byte layout. Without that, the two sites can easily
> > drift apart again.
> >
> > Patch 2 backfils test coverage for util/envlist -- there was none --
> > in tests/unit/test-envlist. I verified that the regression case
> > (envlist_unsetenv("FOO") vs. a stored "FOOBAR=...") fails against the
> > pre-fix code and passes after the fix.
> >
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> >
> > Denis V. Lunev (2):
> >   util/envlist: fix prefix-match in envlist_unsetenv() name lookup
> >   tests/unit: add test-envlist covering setenv/unsetenv name matching
> >
> >  tests/unit/meson.build    |   1 +
> >  tests/unit/test-envlist.c | 196 ++++++++++++++++++++++++++++++++++++++
> >  util/envlist.c            |  19 +++-
> >  3 files changed, 212 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/unit/test-envlist.c
> >

I will way until Wednesday to merge this so that the -user maintainers
have a chance to take a look:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] util/envlist: fix prefix-match in name lookup
  2026-05-28  9:24 ` [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev
  2026-06-01 19:12   ` Stefan Hajnoczi
  2026-06-01 19:18   ` Stefan Hajnoczi
@ 2026-06-03 16:51   ` Stefan Hajnoczi
  2 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2026-06-03 16:51 UTC (permalink / raw)
  To: Denis V. Lunev
  Cc: Denis V. Lunev, qemu-devel, Markus Armbruster, Paolo Bonzini

[-- Attachment #1: Type: text/plain, Size: 2187 bytes --]

On Thu, May 28, 2026 at 11:24:48AM +0200, Denis V. Lunev wrote:
> On 5/20/26 23:26, Denis V. Lunev wrote:
> > A bug report against our downstream tree turned out to have its root
> > cause in plain mainstream code: envlist_unsetenv() does a prefix-match
> > lookup that drops the wrong entry when one stored name happens to be a
> > prefix of another. The downstream symptom is specific to our setup and
> > isn't interesting here -- the underlying lookup mistake is the part
> > worth fixing, and it is reachable from a normal qemu-user invocation
> > through the -U command-line option, so the fix belongs upstream.
> >
> > Patch 1 fixes the lookup: each entry now stores its name length at
> > insertion time, and a tiny helper compares with explicit length
> > equality plus memcmp. envlist_setenv()'s self-search was accidentally
> > safe (it included the '=' byte in its strncmp window and that '='
> > served as a boundary) but is converted to the same helper so the name
> > boundary becomes a structural property of the entry rather than a
> > property of its byte layout. Without that, the two sites can easily
> > drift apart again.
> >
> > Patch 2 backfils test coverage for util/envlist -- there was none --
> > in tests/unit/test-envlist. I verified that the regression case
> > (envlist_unsetenv("FOO") vs. a stored "FOOBAR=...") fails against the
> > pre-fix code and passes after the fix.
> >
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> >
> > Denis V. Lunev (2):
> >   util/envlist: fix prefix-match in envlist_unsetenv() name lookup
> >   tests/unit: add test-envlist covering setenv/unsetenv name matching
> >
> >  tests/unit/meson.build    |   1 +
> >  tests/unit/test-envlist.c | 196 ++++++++++++++++++++++++++++++++++++++
> >  util/envlist.c            |  19 +++-
> >  3 files changed, 212 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/unit/test-envlist.c
> >
> ping
> 

Thanks, applied to staging:
https://gitlab.com/qemu-project/qemu/commits/staging

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-06-03 16:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-20 21:26 [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev via qemu development
2026-05-20 21:26 ` [PATCH 1/2] util/envlist: fix prefix-match in envlist_unsetenv() " Denis V. Lunev via qemu development
2026-05-20 21:26 ` [PATCH 2/2] tests/unit: add test-envlist covering setenv/unsetenv name matching Denis V. Lunev via qemu development
2026-05-28  9:24 ` [PATCH 0/2] util/envlist: fix prefix-match in name lookup Denis V. Lunev
2026-06-01 19:12   ` Stefan Hajnoczi
2026-06-01 19:18   ` Stefan Hajnoczi
2026-06-03 16:51   ` Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.