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

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

[Markus Armbruster]

Makes Coverity happy with libcacard/ (for now).

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 | 16 ++++++----------
 libcacard/vreader.c        | 10 ++++------
 libcacard/vscclient.c      |  7 +++----
 4 files changed, 15 insertions(+), 31 deletions(-)

-- 
1.9.0

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

[Markus Armbruster]

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@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 2/7] libcacard: Plug memory leaks around vreader_get_reader_list()

[Markus Armbruster]

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@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 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>
---
 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 4/7] libcacard/vreader: Tighten assertion to clarify intent

[Markus Armbruster]

Bonus: hushes up Coverity.

Signed-off-by: Markus Armbruster <armbru@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 5/7] libcacard: Convert two leftover realloc() to GLib

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 libcacard/cac.c            | 13 ++-----------
 libcacard/vcard_emul_nss.c |  6 +-----
 2 files changed, 3 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..f98541f 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1178,11 +1178,7 @@ 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_new(VirtualReaderOptions, reader_count);
             }
             opts->vreader = vreaderOpt;
             vreaderOpt = &vreaderOpt[opts->vreader_count];
-- 
1.9.0

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

[Michael Tokarev]

22.05.2014 18:57, Markus Armbruster wrote:

> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> index 692534c..f98541f 100644
> --- a/libcacard/vcard_emul_nss.c
> +++ b/libcacard/vcard_emul_nss.c
> @@ -1178,11 +1178,7 @@ 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_new(VirtualReaderOptions, reader_count);
>              }
>              opts->vreader = vreaderOpt;

This does not look like equivalent code.  It is equivalent
to malloc(), not realloc().  So we'll leak old opts->vreader
on every expansion of the array, and will lose old elements
in it too.

Thanks,

/mjt

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

[Markus Armbruster]

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

> 22.05.2014 18:57, Markus Armbruster wrote:
>
>> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
>> index 692534c..f98541f 100644
>> --- a/libcacard/vcard_emul_nss.c
>> +++ b/libcacard/vcard_emul_nss.c
>> @@ -1178,11 +1178,7 @@ 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_new(VirtualReaderOptions, reader_count);
>>              }
>>              opts->vreader = vreaderOpt;
>
> This does not look like equivalent code.  It is equivalent
> to malloc(), not realloc().  So we'll leak old opts->vreader
> on every expansion of the array, and will lose old elements
> in it too.

Typo, meant g_renew(), will respin.  Thanks!

[Qemu-devel] [PATCH 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>
---
 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 f98541f..83b89e4 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1180,6 +1180,7 @@ vcard_emul_options(const char *args)
                 reader_count += READER_STEP;
                 vreaderOpt = g_new(VirtualReaderOptions, 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 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>
---
 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 83b89e4..2ebe13f 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 0/7] libcacard: A few simple fixes and cleanups

[Alon Levy]

On 05/22/2014 05:57 PM, Markus Armbruster wrote:
> Makes Coverity happy with libcacard/ (for now).
> 
> 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 | 16 ++++++----------
>  libcacard/vreader.c        | 10 ++++------
>  libcacard/vscclient.c      |  7 +++----
>  4 files changed, 15 insertions(+), 31 deletions(-)
> 

Reviewed-by: Alon Levy <alevy@redhat.com>