[v2] Fix use-after-free and make format overflow more difficult

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

Posted by Akihiko Odaki 1 month, 1 week ago

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

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

Posted by Markus Armbruster 1 month, 1 week ago

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

Posted by Daniel P. Berrangé 1 month, 1 week ago

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

Posted by Akihiko Odaki 1 month, 1 week ago

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