[Qemu-devel] [PATCH v2 0/7] libcacard: A few simple fixes and cleanups

[Qemu-devel] [PATCH v2 0/7] libcacard: A few simple fixes and cleanups

[Markus Armbruster]

Makes Coverity happy with libcacard/ (for now).

v2:
- Fix g_new() vs. g_renew() mistake in PATCH 5 [Michael Tokarev]
- Trivially rebased

Markus Armbruster (7):
  libcacard/vscclient: Bury some dead code
  libcacard: Plug memory leaks around vreader_get_reader_list()
  libcacard/vreader: Drop broken recovery from failed assertion
  libcacard/vreader: Tighten assertion to clarify intent
  libcacard: Convert two leftover realloc() to GLib
  libcacard/vcard_emul_nss: Assert vreaderOpt isn't null
  libcacard/vcard_emul_nss: Drop a redundant conditional

 libcacard/cac.c            | 13 ++-----------
 libcacard/vcard_emul_nss.c | 17 +++++++----------
 libcacard/vreader.c        | 10 ++++------
 libcacard/vscclient.c      |  7 +++----
 4 files changed, 16 insertions(+), 31 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v2 1/7] libcacard/vscclient: Bury some dead code

[Markus Armbruster]

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vscclient.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 3477ab3..29f4958 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -502,8 +502,7 @@ do_command(GIOChannel *source,
             if (reader != NULL) {
                 error = vcard_emul_force_card_insert(reader);
                 printf("insert %s, returned %d\n",
-                       reader ? vreader_get_name(reader)
-                       : "invalid reader", error);
+                       vreader_get_name(reader), error);
             } else {
                 printf("no reader by id %u found\n", reader_id);
             }
@@ -515,8 +514,7 @@ do_command(GIOChannel *source,
             if (reader != NULL) {
                 error = vcard_emul_force_card_remove(reader);
                 printf("remove %s, returned %d\n",
-                        reader ? vreader_get_name(reader)
-                        : "invalid reader", error);
+                       vreader_get_name(reader), error);
             } else {
                 printf("no reader by id %u found\n", reader_id);
             }
-- 
1.9.0

[Qemu-devel] [PATCH v2 2/7] libcacard: Plug memory leaks around vreader_get_reader_list()

[Markus Armbruster]

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vcard_emul_nss.c | 4 ++++
 libcacard/vscclient.c      | 1 +
 2 files changed, 5 insertions(+)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index e2b196d..692534c 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -433,11 +433,13 @@ vcard_emul_find_vreader_from_slot(PK11SlotInfo *slot)
         VReader *reader = vreader_list_get_reader(current_entry);
         VReaderEmul *reader_emul = vreader_get_private(reader);
         if (reader_emul->slot == slot) {
+            vreader_list_delete(reader_list);
             return reader;
         }
         vreader_free(reader);
     }
 
+    vreader_list_delete(reader_list);
     return NULL;
 }
 
@@ -1059,6 +1061,8 @@ vcard_emul_replay_insertion_events(void)
         next_entry = vreader_list_get_next(current_entry);
         vreader_queue_card_event(vreader);
     }
+
+    vreader_list_delete(list);
 }
 
 /*
diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 29f4958..f2a753a 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -570,6 +570,7 @@ do_command(GIOChannel *source,
                        "CARD_PRESENT" : "            ",
                        vreader_get_name(reader));
             }
+            vreader_list_delete(list);
         } else if (*string != 0) {
             printf("valid commands:\n");
             printf("insert [reader_id]\n");
-- 
1.9.0

[Qemu-devel] [PATCH v2 3/7] libcacard/vreader: Drop broken recovery from failed assertion

[Markus Armbruster]

We suppress some code when we got unexpected status and assertion
checking is off:

     assert(card_status == VCARD_DONE);
     if (card_status == VCARD_DONE) {
         int size = MIN(*receive_buf_len, response->b_total_len);
         memcpy(receive_buf, response->b_data, size);
         *receive_buf_len = size;
    }

Such "recovery" is of dubious value even when it works.  This one
doesn't: it fails to assign to receive_buf[] and *receive_buf_len,
which the callers expect.

Make the code unconditional.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vreader.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/libcacard/vreader.c b/libcacard/vreader.c
index 7720295..c966bb3 100644
--- a/libcacard/vreader.c
+++ b/libcacard/vreader.c
@@ -284,11 +284,9 @@ vreader_xfr_bytes(VReader *reader,
         }
     }
     assert(card_status == VCARD_DONE);
-    if (card_status == VCARD_DONE) {
-        int size = MIN(*receive_buf_len, response->b_total_len);
-        memcpy(receive_buf, response->b_data, size);
-        *receive_buf_len = size;
-    }
+    int size = MIN(*receive_buf_len, response->b_total_len);
+    memcpy(receive_buf, response->b_data, size);
+    *receive_buf_len = size;
     vcard_response_delete(response);
     vcard_apdu_delete(apdu);
     vcard_free(card); /* free our reference */
-- 
1.9.0

[Qemu-devel] [PATCH v2 4/7] libcacard/vreader: Tighten assertion to clarify intent

[Markus Armbruster]

Bonus: hushes up Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vreader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libcacard/vreader.c b/libcacard/vreader.c
index c966bb3..3a8f81a 100644
--- a/libcacard/vreader.c
+++ b/libcacard/vreader.c
@@ -283,7 +283,7 @@ vreader_xfr_bytes(VReader *reader,
                   response->b_sw2, response->b_len, response->b_total_len);
         }
     }
-    assert(card_status == VCARD_DONE);
+    assert(card_status == VCARD_DONE && response);
     int size = MIN(*receive_buf_len, response->b_total_len);
     memcpy(receive_buf, response->b_data, size);
     *receive_buf_len = size;
-- 
1.9.0

[Qemu-devel] [PATCH v2 5/7] libcacard: Convert two leftover realloc() to GLib

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/cac.c            | 13 ++-----------
 libcacard/vcard_emul_nss.c |  7 ++-----
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/libcacard/cac.c b/libcacard/cac.c
index 74ef3e3..05ce8a2 100644
--- a/libcacard/cac.c
+++ b/libcacard/cac.c
@@ -169,17 +169,8 @@ cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
         }
         size = apdu->a_Lc;
 
-        sign_buffer = realloc(pki_applet->sign_buffer,
-                      pki_applet->sign_buffer_len+size);
-        if (sign_buffer == NULL) {
-            g_free(pki_applet->sign_buffer);
-            pki_applet->sign_buffer = NULL;
-            pki_applet->sign_buffer_len = 0;
-            *response = vcard_make_response(
-                            VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE);
-            ret = VCARD_DONE;
-            break;
-        }
+        sign_buffer = g_realloc(pki_applet->sign_buffer,
+                                pki_applet->sign_buffer_len + size);
         memcpy(sign_buffer+pki_applet->sign_buffer_len, apdu->a_body, size);
         size += pki_applet->sign_buffer_len;
         switch (apdu->a_p1) {
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 692534c..2048917 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1178,11 +1178,8 @@ vcard_emul_options(const char *args)
 
             if (opts->vreader_count >= reader_count) {
                 reader_count += READER_STEP;
-                vreaderOpt = realloc(opts->vreader,
-                                reader_count * sizeof(*vreaderOpt));
-                if (vreaderOpt == NULL) {
-                    return opts; /* we're done */
-                }
+                vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
+                                     reader_count);
             }
             opts->vreader = vreaderOpt;
             vreaderOpt = &vreaderOpt[opts->vreader_count];
-- 
1.9.0

[Qemu-devel] [PATCH v2 6/7] libcacard/vcard_emul_nss: Assert vreaderOpt isn't null

[Markus Armbruster]

It's not locally obvious, and Coverity can't see it either.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vcard_emul_nss.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 2048917..4f55e44 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1181,6 +1181,7 @@ vcard_emul_options(const char *args)
                 vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
                                      reader_count);
             }
+            assert(vreaderOpt);
             opts->vreader = vreaderOpt;
             vreaderOpt = &vreaderOpt[opts->vreader_count];
             vreaderOpt->name = g_strndup(name, name_length);
-- 
1.9.0

[Qemu-devel] [PATCH v2 7/7] libcacard/vcard_emul_nss: Drop a redundant conditional

[Markus Armbruster]

Bailing out when PK11_FindGenericObjects() returns null ensures the
loop that follows it executes at least once.  The "loop did not
execute" test right after it is useless.  Drop it.

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vcard_emul_nss.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 4f55e44..8f8e9e0 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -618,11 +618,6 @@ vcard_emul_mirror_card(VReader *vreader)
         cert_count++;
     }
 
-    if (cert_count == 0) {
-        PK11_DestroyGenericObjects(firstObj);
-        return NULL;
-    }
-
     /* allocate the arrays */
     vcard_emul_alloc_arrays(&certs, &cert_len, &keys, cert_count);
 
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 6/7] libcacard/vcard_emul_nss: Assert vreaderOpt isn't null

[Michael Tokarev]

23.05.2014 15:24, Markus Armbruster пишет:
> It's not locally obvious, and Coverity can't see it either.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Alon Levy <alevy@redhat.com>
> ---
>  libcacard/vcard_emul_nss.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> index 2048917..4f55e44 100644
> --- a/libcacard/vcard_emul_nss.c
> +++ b/libcacard/vcard_emul_nss.c
> @@ -1181,6 +1181,7 @@ vcard_emul_options(const char *args)
>                  vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
>                                       reader_count);
>              }
> +            assert(vreaderOpt);
>              opts->vreader = vreaderOpt;
>              vreaderOpt = &vreaderOpt[opts->vreader_count];
>              vreaderOpt->name = g_strndup(name, name_length);

Shouldn't the assignment be moved up one line into the if {}
statement instead?

Sigh, thats a second comment about this code... :)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v2 6/7] libcacard/vcard_emul_nss: Assert vreaderOpt isn't null

[Michael Tokarev]

23.05.2014 22:09, Michael Tokarev wrote:
> 23.05.2014 15:24, Markus Armbruster wrote:
>> It's not locally obvious, and Coverity can't see it either.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Alon Levy <alevy@redhat.com>
>> ---
>>  libcacard/vcard_emul_nss.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
>> index 2048917..4f55e44 100644
>> --- a/libcacard/vcard_emul_nss.c
>> +++ b/libcacard/vcard_emul_nss.c
>> @@ -1181,6 +1181,7 @@ vcard_emul_options(const char *args)
>>                  vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
>>                                       reader_count);
>>              }
>> +            assert(vreaderOpt);
>>              opts->vreader = vreaderOpt;
>>              vreaderOpt = &vreaderOpt[opts->vreader_count];
>>              vreaderOpt->name = g_strndup(name, name_length);
> 
> Shouldn't the assignment be moved up one line into the if {}
> statement instead?

Actually it looks like this code is just buggy, it works for just
one iteration.  Because at this place, vreaderOpts will be non-NULL
only if we expanded the array.  If we didn't (we do that in
READER_STEP increments), we'll be assigning NULL to opts->vreader,
because vreaderOpt will _really_ be NULL here.

One good example when setting initial values like this (for vreaderOpts)
is a Bad Idea (tm).  If it weren't needlessly NULL initially, compiler
was able to tell us about using uninitialized variable.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v2 6/7] libcacard/vcard_emul_nss: Assert vreaderOpt isn't null

[Michael Tokarev]

23.05.2014 22:16, Michael Tokarev пишет:
> 23.05.2014 22:09, Michael Tokarev wrote:
>> 23.05.2014 15:24, Markus Armbruster wrote:
>>> It's not locally obvious, and Coverity can't see it either.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Alon Levy <alevy@redhat.com>
>>> ---
>>>  libcacard/vcard_emul_nss.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
>>> index 2048917..4f55e44 100644
>>> --- a/libcacard/vcard_emul_nss.c
>>> +++ b/libcacard/vcard_emul_nss.c
>>> @@ -1181,6 +1181,7 @@ vcard_emul_options(const char *args)
>>>                  vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
>>>                                       reader_count);
>>>              }
>>> +            assert(vreaderOpt);
>>>              opts->vreader = vreaderOpt;
>>>              vreaderOpt = &vreaderOpt[opts->vreader_count];
>>>              vreaderOpt->name = g_strndup(name, name_length);
>>
>> Shouldn't the assignment be moved up one line into the if {}
>> statement instead?
> 
> Actually it looks like this code is just buggy, it works for just
> one iteration.  Because at this place, vreaderOpts will be non-NULL
> only if we expanded the array.  If we didn't (we do that in
> READER_STEP increments), we'll be assigning NULL to opts->vreader,
> because vreaderOpt will _really_ be NULL here.

So, the real fix is:

1) drop = NULL at declaration of vreaderOpt;
2) do not mention vreaderOpt inside the if{} statement at
   all, we don't need indirection there;
3) drop this opts->vreader assignment

and ofcourse do not add the assert as in the patch above ;)

/mjt

> One good example when setting initial values like this (for vreaderOpts)
> is a Bad Idea (tm).  If it weren't needlessly NULL initially, compiler
> was able to tell us about using uninitialized variable.
> 
> Thanks,
> 
> /mjt
> 

Re: [Qemu-devel] [PATCH v2 0/7] libcacard: A few simple fixes and cleanups

[Michael Tokarev]

23.05.2014 15:24, Markus Armbruster wrote:
> Makes Coverity happy with libcacard/ (for now).
> 
> v2:
> - Fix g_new() vs. g_renew() mistake in PATCH 5 [Michael Tokarev]
> - Trivially rebased

> Markus Armbruster (7):
>   libcacard/vscclient: Bury some dead code
>   libcacard: Plug memory leaks around vreader_get_reader_list()
>   libcacard/vreader: Drop broken recovery from failed assertion
>   libcacard/vreader: Tighten assertion to clarify intent
>   libcacard: Convert two leftover realloc() to GLib
>   libcacard/vcard_emul_nss: Assert vreaderOpt isn't null
>   libcacard/vcard_emul_nss: Drop a redundant conditional

Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>

Except of the vreaderOpt assertion patch which I commented
separately.

I think this can easily go to the -trivial tree, as all the
changes are really trivial.  Including the bugfix, for which
I can send a patch myself, again, as described in my previous
reply.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v2 0/7] libcacard: A few simple fixes and cleanups

[Michael Tokarev]

23.05.2014 22:20, Michael Tokarev wrote:
> 23.05.2014 15:24, Markus Armbruster wrote:
>> Makes Coverity happy with libcacard/ (for now).
>>
>> v2:
>> - Fix g_new() vs. g_renew() mistake in PATCH 5 [Michael Tokarev]
>> - Trivially rebased
> 
>> Markus Armbruster (7):
>>   libcacard/vscclient: Bury some dead code
>>   libcacard: Plug memory leaks around vreader_get_reader_list()
>>   libcacard/vreader: Drop broken recovery from failed assertion
>>   libcacard/vreader: Tighten assertion to clarify intent
>>   libcacard: Convert two leftover realloc() to GLib
>>   libcacard/vcard_emul_nss: Assert vreaderOpt isn't null
>>   libcacard/vcard_emul_nss: Drop a redundant conditional
> 
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> 
> Except of the vreaderOpt assertion patch which I commented
> separately.
> 
> I think this can easily go to the -trivial tree, as all the
> changes are really trivial.  Including the bugfix, for which
> I can send a patch myself, again, as described in my previous
> reply.

I applied whole series to -trivial, except of this problematic
patch, for which I applied a real fix (will post in another
email).

Good stuff ;)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v2 6/7] libcacard/vcard_emul_nss: Assert vreaderOpt isn't null

[Markus Armbruster]

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

> 23.05.2014 22:16, Michael Tokarev пишет:
>> 23.05.2014 22:09, Michael Tokarev wrote:
>>> 23.05.2014 15:24, Markus Armbruster wrote:
>>>> It's not locally obvious, and Coverity can't see it either.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> Reviewed-by: Alon Levy <alevy@redhat.com>
>>>> ---
>>>>  libcacard/vcard_emul_nss.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
>>>> index 2048917..4f55e44 100644
>>>> --- a/libcacard/vcard_emul_nss.c
>>>> +++ b/libcacard/vcard_emul_nss.c
>>>> @@ -1181,6 +1181,7 @@ vcard_emul_options(const char *args)
>>>>                  vreaderOpt = g_renew(VirtualReaderOptions, opts->vreader,
>>>>                                       reader_count);
>>>>              }
>>>> +            assert(vreaderOpt);
>>>>              opts->vreader = vreaderOpt;
>>>>              vreaderOpt = &vreaderOpt[opts->vreader_count];
>>>>              vreaderOpt->name = g_strndup(name, name_length);
>>>
>>> Shouldn't the assignment be moved up one line into the if {}
>>> statement instead?
>> 
>> Actually it looks like this code is just buggy, it works for just
>> one iteration.  Because at this place, vreaderOpts will be non-NULL
>> only if we expanded the array.  If we didn't (we do that in
>> READER_STEP increments), we'll be assigning NULL to opts->vreader,
>> because vreaderOpt will _really_ be NULL here.

You're right.  When I saw the conditional realloc, I jumped to convince
myself that it's always executed in the first loop iteration, but missed
the fact that vreaderOpt is reset to null on *every* iteration.

> So, the real fix is:
>
> 1) drop = NULL at declaration of vreaderOpt;
> 2) do not mention vreaderOpt inside the if{} statement at
>    all, we don't need indirection there;
> 3) drop this opts->vreader assignment
>
> and ofcourse do not add the assert as in the patch above ;)

I'll review it.  Thanks!