[PATCH v2 0/4] Fix use-after-free and make format overflow more difficult

[PATCH v2 0/4] Fix use-after-free and make format overflow more difficult

[Akihiko Odaki]

nvme-ns has a use-after-free of a formatted string, so fix it by
embedding a fixed-length buffer to the object. Embedding a buffer lets
me avoid a chore to add a function to call g_free().

But I don't want to worry about a buffer overflow, so let the compiler
check that the buffer won't overflow; C is so restrictive that it cannot
enforce the existence of g_free(). Compilers can check the length of
formatted string on the other hand.

Then GCC started complaining about buffer overflow, so let's treat them.
Fortunately, the potential buffer overflows it detected are not
user-facing or very subtle. Treating them by growing buffers can improve
robustness with practically no cost.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
Changes in v2:
- Rebased.
- Changed to use g_strdup_printf() in patch
  "contrib/elf2dmp: Grow PDB URL buffer".
- Link to v1: https://lore.kernel.org/qemu-devel/20260125-nvme-v1-0-0658c31fade9@rsg.ci.i.u-tokyo.ac.jp

---
Akihiko Odaki (4):
      contrib/elf2dmp: Grow PDB URL buffer
      vfio/pci: Grow buffer in vfio_pci_host_match()
      tests: Grow buffers for double string
      meson: Add -Wformat-overflow=2

 meson.build                              |  1 +
 contrib/elf2dmp/main.c                   | 32 +++++++++++++++-----------------
 hw/vfio/pci.c                            |  2 +-
 tests/unit/test-qobject-input-visitor.c  |  2 +-
 tests/unit/test-qobject-output-visitor.c |  2 +-
 5 files changed, 19 insertions(+), 20 deletions(-)
---
base-commit: afe653676dc6dfd49f0390239ff90b2f0052c2b8
change-id: 20260125-nvme-b4661e0a409e

Best regards,
--  
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

[PATCH v2 1/4] contrib/elf2dmp: Grow PDB URL buffer

[Akihiko Odaki]

The buffers used to construct a PDB URL overflow when the "age" property
is greater than 0xf, so grow it. This also simplifies the logic of the
URL construction to use one buffer instead of two to avoid the chore to
synchronize the sizes of two buffers.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 contrib/elf2dmp/main.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index d046a72ae67f..a62abadcc049 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -494,18 +494,6 @@ static bool pe_check_pdb_name(uint64_t base, void *start_addr,
     return !strcmp(pdb_name, PDB_NAME);
 }
 
-static void pe_get_pdb_symstore_hash(OMFSignatureRSDS *rsds, char *hash)
-{
-    sprintf(hash, "%.08x%.04x%.04x%.02x%.02x", rsds->guid.a, rsds->guid.b,
-            rsds->guid.c, rsds->guid.d[0], rsds->guid.d[1]);
-    hash += 20;
-    for (unsigned int i = 0; i < 6; i++, hash += 2) {
-        sprintf(hash, "%.02x", rsds->guid.e[i]);
-    }
-
-    sprintf(hash, "%.01x", rsds->age);
-}
-
 int main(int argc, char *argv[])
 {
     int err = 1;
@@ -517,9 +505,7 @@ int main(int argc, char *argv[])
     uint64_t KernBase;
     void *nt_start_addr = NULL;
     WinDumpHeader64 header;
-    char pdb_hash[34];
-    char pdb_url[] = SYM_URL_BASE PDB_NAME
-        "/0123456789ABCDEF0123456789ABCDEFx/" PDB_NAME;
+    g_autofree char *pdb_url = NULL;
     struct pdb_reader pdb;
     uint64_t KdDebuggerDataBlock;
     KDDEBUGGER_DATA64 *kdbg;
@@ -583,9 +569,21 @@ int main(int argc, char *argv[])
     printf("KernBase = 0x%016"PRIx64", signature is \'%.2s\'\n", KernBase,
             (char *)nt_start_addr);
 
-    pe_get_pdb_symstore_hash(&rsds, pdb_hash);
+    pdb_url = g_strdup_printf("%s"
+                              "%.08x%.04x%.04x"
+                              "%.02x%.02x"
+                              "%.02x%.02x"
+                              "%.02x%.02x"
+                              "%.02x%.02x%.01x"
+                              "%s",
+                              SYM_URL_BASE PDB_NAME "/",
+                              rsds.guid.a, rsds.guid.b, rsds.guid.c,
+                              rsds.guid.d[0], rsds.guid.d[1],
+                              rsds.guid.e[0], rsds.guid.e[1],
+                              rsds.guid.e[2], rsds.guid.e[3],
+                              rsds.guid.e[4], rsds.guid.e[5], rsds.age,
+                              "/" PDB_NAME);
 
-    sprintf(pdb_url, "%s%s/%s/%s", SYM_URL_BASE, PDB_NAME, pdb_hash, PDB_NAME);
     printf("PDB URL is %s\n", pdb_url);
 
     if (!download_url(PDB_NAME, pdb_url)) {

-- 
2.53.0

[PATCH v2 2/4] vfio/pci: Grow buffer in vfio_pci_host_match()

[Akihiko Odaki]

Ensure the buffer in vfio_pci_host_match() will not overflow even when
an invalid addr parameter is provided.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c89f3fbea348..94c174a773fb 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2739,7 +2739,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
 
 bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
 {
-    char tmp[13];
+    char tmp[36];
 
     sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
             addr->bus, addr->slot, addr->function);

-- 
2.53.0

[PATCH v2 3/4] tests: Grow buffers for double string

[Akihiko Odaki]

A string that represents a double can be long if it is an exponentially
large number.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 tests/unit/test-qobject-input-visitor.c  | 2 +-
 tests/unit/test-qobject-output-visitor.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
index 84bdcdf702e0..baff9243313c 100644
--- a/tests/unit/test-qobject-input-visitor.c
+++ b/tests/unit/test-qobject-input-visitor.c
@@ -583,7 +583,7 @@ static void test_visitor_in_list_struct(TestInputVisitorData *data,
 
     i = 0;
     for (num_list = arrs->number; num_list; num_list = num_list->next) {
-        char expected[32], actual[32];
+        char expected[318], actual[318];
 
         sprintf(expected, "%.6f", (double)i / 3);
         sprintf(actual, "%.6f", num_list->value);
diff --git a/tests/unit/test-qobject-output-visitor.c b/tests/unit/test-qobject-output-visitor.c
index 407ab9ed505a..ae05a726f775 100644
--- a/tests/unit/test-qobject-output-visitor.c
+++ b/tests/unit/test-qobject-output-visitor.c
@@ -571,7 +571,7 @@ static void test_visitor_out_list_struct(TestOutputVisitorData *data,
     i = 0;
     QLIST_FOREACH_ENTRY(qlist, e) {
         QNum *qvalue = qobject_to(QNum, qlist_entry_obj(e));
-        char expected[32], actual[32];
+        char expected[318], actual[318];
 
         g_assert(qvalue);
         sprintf(expected, "%.6f", (double)i / 3);

-- 
2.53.0

[PATCH v2 4/4] meson: Add -Wformat-overflow=2

[Akihiko Odaki]

https://gcc.gnu.org/onlinedocs/gcc-15.2.0/gcc/Warning-Options.html
> Level 2 warns also about calls that might overflow the destination
> buffer given an argument of sufficient length or magnitude. At level
> 2, unknown numeric arguments are assumed to have the minimum
> representable value for signed types with a precision greater than 1,
> and the maximum representable value otherwise. Unknown string
> arguments whose length cannot be assumed to be bounded either by the
> directive’s precision, or by a finite set of string literals they may
> evaluate to, or the character array they may point to, are assumed to
> be 1 character long.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meson.build b/meson.build
index 414c8ea7e280..cf50bc492f9c 100644
--- a/meson.build
+++ b/meson.build
@@ -692,6 +692,7 @@ warn_flags = [
   '-Wempty-body',
   '-Wendif-labels',
   '-Wexpansion-to-defined',
+  '-Wformat-overflow=2',
   '-Wformat-security',
   '-Wformat-y2k',
   '-Wignored-qualifiers',

-- 
2.53.0

Re: [PATCH v2 3/4] tests: Grow buffers for double string

[Markus Armbruster]

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> A string that represents a double can be long if it is an exponentially
> large number.
>
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  tests/unit/test-qobject-input-visitor.c  | 2 +-
>  tests/unit/test-qobject-output-visitor.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
> index 84bdcdf702e0..baff9243313c 100644
> --- a/tests/unit/test-qobject-input-visitor.c
> +++ b/tests/unit/test-qobject-input-visitor.c
> @@ -583,7 +583,7 @@ static void test_visitor_in_list_struct(TestInputVisitorData *data,
>  
>      i = 0;
>      for (num_list = arrs->number; num_list; num_list = num_list->next) {
> -        char expected[32], actual[32];
> +        char expected[318], actual[318];

Where does 318 come from?

>  
>          sprintf(expected, "%.6f", (double)i / 3);
>          sprintf(actual, "%.6f", num_list->value);
           g_assert_cmpstr(expected, ==, actual);
           i++;
       }

Existing code is safe, because the numbers run from 0, 1.0/3, ...,
31.0/3.

Its purpose is to check the input visitor parses number arrays
correctly.  Doing it this way is questionable.  Elsewhere in this file,
we get away with the equivalent of

           g_assert_cmpfloat(num_list->value, ==, (double)i / 3);

Yes, double can't represent the fractions exactly, but if we're
concerned about that, we should test the difference is less than
epsilon, or simply use representable values.

> diff --git a/tests/unit/test-qobject-output-visitor.c b/tests/unit/test-qobject-output-visitor.c
> index 407ab9ed505a..ae05a726f775 100644
> --- a/tests/unit/test-qobject-output-visitor.c
> +++ b/tests/unit/test-qobject-output-visitor.c
> @@ -571,7 +571,7 @@ static void test_visitor_out_list_struct(TestOutputVisitorData *data,
>      i = 0;
>      QLIST_FOREACH_ENTRY(qlist, e) {
>          QNum *qvalue = qobject_to(QNum, qlist_entry_obj(e));
> -        char expected[32], actual[32];
> +        char expected[318], actual[318];
>  
>          g_assert(qvalue);
>          sprintf(expected, "%.6f", (double)i / 3);

Likewise.

Re: [PATCH v2 3/4] tests: Grow buffers for double string

[Daniel P. Berrangé]

On Mon, Mar 02, 2026 at 12:52:10PM +0100, Markus Armbruster wrote:
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
> 
> > A string that represents a double can be long if it is an exponentially
> > large number.
> >
> > Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> > ---
> >  tests/unit/test-qobject-input-visitor.c  | 2 +-
> >  tests/unit/test-qobject-output-visitor.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
> > index 84bdcdf702e0..baff9243313c 100644
> > --- a/tests/unit/test-qobject-input-visitor.c
> > +++ b/tests/unit/test-qobject-input-visitor.c
> > @@ -583,7 +583,7 @@ static void test_visitor_in_list_struct(TestInputVisitorData *data,
> >  
> >      i = 0;
> >      for (num_list = arrs->number; num_list; num_list = num_list->next) {
> > -        char expected[32], actual[32];
> > +        char expected[318], actual[318];
> 
> Where does 318 come from?

If we're concerned about buffer sizes being too short, then that
is a strong sign we should be using g_strdup_printf instead of
sprintf with a bigger magic size.

As you say below though, it is better if we eliminate the string
formatting entirely here since it is irrelevant for the goals of
this test.

> 
> >  
> >          sprintf(expected, "%.6f", (double)i / 3);
> >          sprintf(actual, "%.6f", num_list->value);
>            g_assert_cmpstr(expected, ==, actual);
>            i++;
>        }
> 
> Existing code is safe, because the numbers run from 0, 1.0/3, ...,
> 31.0/3.
> 
> Its purpose is to check the input visitor parses number arrays
> correctly.  Doing it this way is questionable.  Elsewhere in this file,
> we get away with the equivalent of
> 
>            g_assert_cmpfloat(num_list->value, ==, (double)i / 3);
> 
> Yes, double can't represent the fractions exactly, but if we're
> concerned about that, we should test the difference is less than
> epsilon, or simply use representable values.
> 
> > diff --git a/tests/unit/test-qobject-output-visitor.c b/tests/unit/test-qobject-output-visitor.c
> > index 407ab9ed505a..ae05a726f775 100644
> > --- a/tests/unit/test-qobject-output-visitor.c
> > +++ b/tests/unit/test-qobject-output-visitor.c
> > @@ -571,7 +571,7 @@ static void test_visitor_out_list_struct(TestOutputVisitorData *data,
> >      i = 0;
> >      QLIST_FOREACH_ENTRY(qlist, e) {
> >          QNum *qvalue = qobject_to(QNum, qlist_entry_obj(e));
> > -        char expected[32], actual[32];
> > +        char expected[318], actual[318];
> >  
> >          g_assert(qvalue);
> >          sprintf(expected, "%.6f", (double)i / 3);
> 
> Likewise.
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v2 3/4] tests: Grow buffers for double string

[Akihiko Odaki]

On 2026/03/02 20:57, Daniel P. Berrangé wrote:
> On Mon, Mar 02, 2026 at 12:52:10PM +0100, Markus Armbruster wrote:
>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>>
>>> A string that represents a double can be long if it is an exponentially
>>> large number.
>>>
>>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>> ---
>>>   tests/unit/test-qobject-input-visitor.c  | 2 +-
>>>   tests/unit/test-qobject-output-visitor.c | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
>>> index 84bdcdf702e0..baff9243313c 100644
>>> --- a/tests/unit/test-qobject-input-visitor.c
>>> +++ b/tests/unit/test-qobject-input-visitor.c
>>> @@ -583,7 +583,7 @@ static void test_visitor_in_list_struct(TestInputVisitorData *data,
>>>   
>>>       i = 0;
>>>       for (num_list = arrs->number; num_list; num_list = num_list->next) {
>>> -        char expected[32], actual[32];
>>> +        char expected[318], actual[318];
>>
>> Where does 318 come from?

The compiler told me the number.

> 
> If we're concerned about buffer sizes being too short, then that
> is a strong sign we should be using g_strdup_printf instead of
> sprintf with a bigger magic size.
> 
> As you say below though, it is better if we eliminate the string
> formatting entirely here since it is irrelevant for the goals of
> this test.
> 
>>
>>>   
>>>           sprintf(expected, "%.6f", (double)i / 3);
>>>           sprintf(actual, "%.6f", num_list->value);
>>             g_assert_cmpstr(expected, ==, actual);
>>             i++;
>>         }
>>
>> Existing code is safe, because the numbers run from 0, 1.0/3, ...,
>> 31.0/3.
>>
>> Its purpose is to check the input visitor parses number arrays
>> correctly.  Doing it this way is questionable.  Elsewhere in this file,
>> we get away with the equivalent of
>>
>>             g_assert_cmpfloat(num_list->value, ==, (double)i / 3);
>>
>> Yes, double can't represent the fractions exactly, but if we're
>> concerned about that, we should test the difference is less than
>> epsilon, or simply use representable values.

I will replace them with representable values.

Regards,
Akihiko Odaki

>>
>>> diff --git a/tests/unit/test-qobject-output-visitor.c b/tests/unit/test-qobject-output-visitor.c
>>> index 407ab9ed505a..ae05a726f775 100644
>>> --- a/tests/unit/test-qobject-output-visitor.c
>>> +++ b/tests/unit/test-qobject-output-visitor.c
>>> @@ -571,7 +571,7 @@ static void test_visitor_out_list_struct(TestOutputVisitorData *data,
>>>       i = 0;
>>>       QLIST_FOREACH_ENTRY(qlist, e) {
>>>           QNum *qvalue = qobject_to(QNum, qlist_entry_obj(e));
>>> -        char expected[32], actual[32];
>>> +        char expected[318], actual[318];
>>>   
>>>           g_assert(qvalue);
>>>           sprintf(expected, "%.6f", (double)i / 3);
>>
>> Likewise.
>>
> 
> With regards,
> Daniel

Re: [PATCH v2 2/4] vfio/pci: Grow buffer in vfio_pci_host_match()

[Alex Williamson]

On Mon, 02 Mar 2026 16:48:09 +0900
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:

> Ensure the buffer in vfio_pci_host_match() will not overflow even when
> an invalid addr parameter is provided.

This commit log could be much more straightforward.  Something like:

    Each field of PCIHostDeviceAddress is an unsigned int, therefore
    while a valid address is limited to 13 characters, an invalid
    address could exceed the specified format, up to:

        ffffffff:ffffffff:ffffffff.ffffffff<NUL>

    This requires 36 characters with the terminator.

With that:

Reviewed-by: Alex Williamson <alex.williamson@nvidia.com>
    

> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  hw/vfio/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c89f3fbea348..94c174a773fb 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2739,7 +2739,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
>  
>  bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
>  {
> -    char tmp[13];
> +    char tmp[36];
>  
>      sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
>              addr->bus, addr->slot, addr->function);
>