[Qemu-trivial] [PATCH] libcacard: fix wrong array expansion logic

[Qemu-trivial] [PATCH] libcacard: fix wrong array expansion logic

[Michael Tokarev]

The currrent code in libcacard/vcard_emul_nss.c:vcard_emul_options()
has a weird bug in variable usage around expanding opts->vreader
array.

There's a helper variable, vreaderOpt, which is first needlessly
initialized to NULL, next, conditionally, only we have to expand
opts->vreader, receives array expansion from g_renew() (initially
realloc), and next, even if we don't actually perform expansion,
the value of this variable is assigned to the actual array,
opts->vreader, which was supposed to be expanded.

So, since we expand the array by READER_STEP increments, only
once in READER_STEP (=4) the code will work, in other 3/4 times
it will fail badly.

Fix this by not using this temp variable when expanding the
array, and by dropping the useless =NULL initializer too -
if it wasn't in place initially, compiler warned us about
this problem at the beginning.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 libcacard/vcard_emul_nss.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index b7db51d..8462aef 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1149,7 +1149,7 @@ vcard_emul_options(const char *args)
             char type_str[100];
             VCardEmulType type;
             int count, i;
-            VirtualReaderOptions *vreaderOpt = NULL;
+            VirtualReaderOptions *vreaderOpt;
 
             args = strip(args + 5);
             if (*args != '(') {
@@ -1173,11 +1173,10 @@ vcard_emul_options(const char *args)
 
             if (opts->vreader_count >= reader_count) {
                 reader_count += READER_STEP;
-                vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
-                                     reader_count);
+                opts->vreader = g_renew(VirtualReaderOptions, opts->vreader,
+                                        reader_count);
             }
-            opts->vreader = vreaderOpt;
-            vreaderOpt = &vreaderOpt[opts->vreader_count];
+            vreaderOpt = &opts->vreader[opts->vreader_count];
             vreaderOpt->name = g_strndup(name, name_length);
             vreaderOpt->vname = g_strndup(vname, vname_length);
             vreaderOpt->card_type = type;
-- 
1.7.10.4

[Qemu-devel] [PATCH] libcacard: fix wrong array expansion logic

[Michael Tokarev]

The currrent code in libcacard/vcard_emul_nss.c:vcard_emul_options()
has a weird bug in variable usage around expanding opts->vreader
array.

There's a helper variable, vreaderOpt, which is first needlessly
initialized to NULL, next, conditionally, only we have to expand
opts->vreader, receives array expansion from g_renew() (initially
realloc), and next, even if we don't actually perform expansion,
the value of this variable is assigned to the actual array,
opts->vreader, which was supposed to be expanded.

So, since we expand the array by READER_STEP increments, only
once in READER_STEP (=4) the code will work, in other 3/4 times
it will fail badly.

Fix this by not using this temp variable when expanding the
array, and by dropping the useless =NULL initializer too -
if it wasn't in place initially, compiler warned us about
this problem at the beginning.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 libcacard/vcard_emul_nss.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index b7db51d..8462aef 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1149,7 +1149,7 @@ vcard_emul_options(const char *args)
             char type_str[100];
             VCardEmulType type;
             int count, i;
-            VirtualReaderOptions *vreaderOpt = NULL;
+            VirtualReaderOptions *vreaderOpt;
 
             args = strip(args + 5);
             if (*args != '(') {
@@ -1173,11 +1173,10 @@ vcard_emul_options(const char *args)
 
             if (opts->vreader_count >= reader_count) {
                 reader_count += READER_STEP;
-                vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
-                                     reader_count);
+                opts->vreader = g_renew(VirtualReaderOptions, opts->vreader,
+                                        reader_count);
             }
-            opts->vreader = vreaderOpt;
-            vreaderOpt = &vreaderOpt[opts->vreader_count];
+            vreaderOpt = &opts->vreader[opts->vreader_count];
             vreaderOpt->name = g_strndup(name, name_length);
             vreaderOpt->vname = g_strndup(vname, vname_length);
             vreaderOpt->card_type = type;
-- 
1.7.10.4

Re: [Qemu-trivial] [Qemu-devel] [PATCH] libcacard: fix wrong array expansion logic

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> The currrent code in libcacard/vcard_emul_nss.c:vcard_emul_options()
> has a weird bug in variable usage around expanding opts->vreader
> array.
>
> There's a helper variable, vreaderOpt, which is first needlessly
> initialized to NULL, next, conditionally, only we have to expand
> opts->vreader, receives array expansion from g_renew() (initially
> realloc), and next, even if we don't actually perform expansion,

I don't get the "(initially realloc)" part.  The sentence makes sense to
me just fine without it, though.

> the value of this variable is assigned to the actual array,
> opts->vreader, which was supposed to be expanded.
>
> So, since we expand the array by READER_STEP increments, only
> once in READER_STEP (=4) the code will work, in other 3/4 times
> it will fail badly.
>
> Fix this by not using this temp variable when expanding the
> array, and by dropping the useless =NULL initializer too -
> if it wasn't in place initially, compiler warned us about

"would have warned us"?

> this problem at the beginning.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  libcacard/vcard_emul_nss.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> index b7db51d..8462aef 100644
> --- a/libcacard/vcard_emul_nss.c
> +++ b/libcacard/vcard_emul_nss.c
> @@ -1149,7 +1149,7 @@ vcard_emul_options(const char *args)
>              char type_str[100];
>              VCardEmulType type;
>              int count, i;
> -            VirtualReaderOptions *vreaderOpt = NULL;
> +            VirtualReaderOptions *vreaderOpt;
>  
>              args = strip(args + 5);
>              if (*args != '(') {
> @@ -1173,11 +1173,10 @@ vcard_emul_options(const char *args)
>  
>              if (opts->vreader_count >= reader_count) {
>                  reader_count += READER_STEP;
> -                vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
> -                                     reader_count);
> +                opts->vreader = g_renew(VirtualReaderOptions, opts->vreader,
> +                                        reader_count);
>              }
> -            opts->vreader = vreaderOpt;
> -            vreaderOpt = &vreaderOpt[opts->vreader_count];
> +            vreaderOpt = &opts->vreader[opts->vreader_count];
>              vreaderOpt->name = g_strndup(name, name_length);
>              vreaderOpt->vname = g_strndup(vname, vname_length);
>              vreaderOpt->card_type = type;

Much more straightforward now.  Thanks!

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] libcacard: fix wrong array expansion logic

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> The currrent code in libcacard/vcard_emul_nss.c:vcard_emul_options()
> has a weird bug in variable usage around expanding opts->vreader
> array.
>
> There's a helper variable, vreaderOpt, which is first needlessly
> initialized to NULL, next, conditionally, only we have to expand
> opts->vreader, receives array expansion from g_renew() (initially
> realloc), and next, even if we don't actually perform expansion,

I don't get the "(initially realloc)" part.  The sentence makes sense to
me just fine without it, though.

> the value of this variable is assigned to the actual array,
> opts->vreader, which was supposed to be expanded.
>
> So, since we expand the array by READER_STEP increments, only
> once in READER_STEP (=4) the code will work, in other 3/4 times
> it will fail badly.
>
> Fix this by not using this temp variable when expanding the
> array, and by dropping the useless =NULL initializer too -
> if it wasn't in place initially, compiler warned us about

"would have warned us"?

> this problem at the beginning.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  libcacard/vcard_emul_nss.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> index b7db51d..8462aef 100644
> --- a/libcacard/vcard_emul_nss.c
> +++ b/libcacard/vcard_emul_nss.c
> @@ -1149,7 +1149,7 @@ vcard_emul_options(const char *args)
>              char type_str[100];
>              VCardEmulType type;
>              int count, i;
> -            VirtualReaderOptions *vreaderOpt = NULL;
> +            VirtualReaderOptions *vreaderOpt;
>  
>              args = strip(args + 5);
>              if (*args != '(') {
> @@ -1173,11 +1173,10 @@ vcard_emul_options(const char *args)
>  
>              if (opts->vreader_count >= reader_count) {
>                  reader_count += READER_STEP;
> -                vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
> -                                     reader_count);
> +                opts->vreader = g_renew(VirtualReaderOptions, opts->vreader,
> +                                        reader_count);
>              }
> -            opts->vreader = vreaderOpt;
> -            vreaderOpt = &vreaderOpt[opts->vreader_count];
> +            vreaderOpt = &opts->vreader[opts->vreader_count];
>              vreaderOpt->name = g_strndup(name, name_length);
>              vreaderOpt->vname = g_strndup(vname, vname_length);
>              vreaderOpt->card_type = type;

Much more straightforward now.  Thanks!

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-trivial] [Qemu-devel] [PATCH] libcacard: fix wrong array expansion logic

[Michael Tokarev]

26.05.2014 10:25, Markus Armbruster wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> The currrent code in libcacard/vcard_emul_nss.c:vcard_emul_options()
>> has a weird bug in variable usage around expanding opts->vreader
>> array.
>>
>> There's a helper variable, vreaderOpt, which is first needlessly
>> initialized to NULL, next, conditionally, only we have to expand
>> opts->vreader, receives array expansion from g_renew() (initially
>> realloc), and next, even if we don't actually perform expansion,
> 
> I don't get the "(initially realloc)" part.  The sentence makes sense to
> me just fine without it, though.

I was in context of your patch which changes realloc() to g_renew().
And I failed to mention that this my patch is on top of yuors,
too - think of this comment as such a mention ;)

>> the value of this variable is assigned to the actual array,
>> opts->vreader, which was supposed to be expanded.
>>
>> So, since we expand the array by READER_STEP increments, only
>> once in READER_STEP (=4) the code will work, in other 3/4 times
>> it will fail badly.
>>
>> Fix this by not using this temp variable when expanding the
>> array, and by dropping the useless =NULL initializer too -
>> if it wasn't in place initially, compiler warned us about
> 
> "would have warned us"?

Oh yeah.  I tried to remember the right English construct, but
failed.  This tense always escpapes my mind for some reason :)


> Much more straightforward now.  Thanks!
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thank you.  I'll fix the comment.  And I'm now ready to push
whole -trivial.

/mjt

Re: [Qemu-devel] [PATCH] libcacard: fix wrong array expansion logic

[Michael Tokarev]

26.05.2014 10:25, Markus Armbruster wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> The currrent code in libcacard/vcard_emul_nss.c:vcard_emul_options()
>> has a weird bug in variable usage around expanding opts->vreader
>> array.
>>
>> There's a helper variable, vreaderOpt, which is first needlessly
>> initialized to NULL, next, conditionally, only we have to expand
>> opts->vreader, receives array expansion from g_renew() (initially
>> realloc), and next, even if we don't actually perform expansion,
> 
> I don't get the "(initially realloc)" part.  The sentence makes sense to
> me just fine without it, though.

I was in context of your patch which changes realloc() to g_renew().
And I failed to mention that this my patch is on top of yuors,
too - think of this comment as such a mention ;)

>> the value of this variable is assigned to the actual array,
>> opts->vreader, which was supposed to be expanded.
>>
>> So, since we expand the array by READER_STEP increments, only
>> once in READER_STEP (=4) the code will work, in other 3/4 times
>> it will fail badly.
>>
>> Fix this by not using this temp variable when expanding the
>> array, and by dropping the useless =NULL initializer too -
>> if it wasn't in place initially, compiler warned us about
> 
> "would have warned us"?

Oh yeah.  I tried to remember the right English construct, but
failed.  This tense always escpapes my mind for some reason :)


> Much more straightforward now.  Thanks!
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thank you.  I'll fix the comment.  And I'm now ready to push
whole -trivial.

/mjt