[Qemu-devel] [PATCHv3 0/4] libcacard fixes

[Qemu-devel] [PATCHv3 0/4] libcacard fixes

[Christophe Fergeau]

Hi,

This is a resend of my series of libcacard fixes, no change since last
version apart from the added "Reviewed-by" tag.

Changes since v2:
- added Reviewed-by: Alon Levy <alevy@redhat.com> to the 4 patches

Changes since v1:
- split first patch into 3 patches, v1 2/2 is unchanged

Christophe Fergeau (4):
  libcacard: fix soft=... parsing in vcard_emul_options
  libcacard: introduce NEXT_TOKEN macro
  libcacard: replace copy_string with strndup
  libcacard: don't leak vcard_emul_alloc_arrays mem

 libcacard/vcard_emul_nss.c |  124 +++++++++++++++++++------------------------
 1 files changed, 55 insertions(+), 69 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCHv3 1/4] libcacard: fix soft=... parsing in vcard_emul_options

[Christophe Fergeau]

The previous parser had copy and paste errors when computing
vname_length and type_params_length, "name" was used instead
of respectively vname and type_params. This led to length that could
be bigger than the input string, and to access out of the array
bounds when trying to copy these strings. valgrind rightfully
complained about this. It also didn't handle empty fields correctly,

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vcard_emul_nss.c |   45 +++++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 184252f..9271f58 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -980,8 +980,6 @@ vcard_emul_options(const char *args)
 {
     int reader_count = 0;
     VCardEmulOptions *opts;
-    char type_str[100];
-    int type_len;
 
     /* Allow the future use of allocating the options structure on the fly */
     memcpy(&options, &default_options, sizeof(options));
@@ -996,18 +994,24 @@ vcard_emul_options(const char *args)
          *       cert_2,cert_3...) */
         if (strncmp(args, "soft=", 5) == 0) {
             const char *name;
+            size_t name_length;
             const char *vname;
+            size_t vname_length;
             const char *type_params;
+            size_t type_params_length;
+            char type_str[100];
             VCardEmulType type;
-            int name_length, vname_length, type_params_length, count, i;
+            int count, i;
             VirtualReaderOptions *vreaderOpt = NULL;
 
             args = strip(args + 5);
             if (*args != '(') {
                 continue;
             }
+            args = strip(args+1);
+
             name = args;
-            args = strpbrk(args + 1, ",)");
+            args = strpbrk(args, ",)");
             if (*args == 0) {
                 break;
             }
@@ -1015,10 +1019,11 @@ vcard_emul_options(const char *args)
                 args++;
                 continue;
             }
+            name_length = args - name;
             args = strip(args+1);
-            name_length = args - name - 2;
+
             vname = args;
-            args = strpbrk(args + 1, ",)");
+            args = strpbrk(args, ",)");
             if (*args == 0) {
                 break;
             }
@@ -1026,13 +1031,10 @@ vcard_emul_options(const char *args)
                 args++;
                 continue;
             }
-            vname_length = args - name - 2;
+            vname_length = args - vname;
             args = strip(args+1);
-            type_len = strpbrk(args, ",)") - args;
-            assert(sizeof(type_str) > type_len);
-            strncpy(type_str, args, type_len);
-            type_str[type_len] = 0;
-            type = vcard_emul_type_from_string(type_str);
+
+            type_params = args;
             args = strpbrk(args, ",)");
             if (*args == 0) {
                 break;
@@ -1041,9 +1043,16 @@ vcard_emul_options(const char *args)
                 args++;
                 continue;
             }
+            type_params_length = args - type_params;
             args = strip(args+1);
+
+            type_params_length = MIN(type_params_length, sizeof(type_str)-1);
+            strncpy(type_str, type_params, type_params_length);
+            type_str[type_params_length] = 0;
+            type = vcard_emul_type_from_string(type_str);
+
             type_params = args;
-            args = strpbrk(args + 1, ",)");
+            args = strpbrk(args, ",)");
             if (*args == 0) {
                 break;
             }
@@ -1051,8 +1060,9 @@ vcard_emul_options(const char *args)
                 args++;
                 continue;
             }
-            type_params_length = args - name;
+            type_params_length = args - type_params;
             args = strip(args+1);
+
             if (*args == 0) {
                 break;
             }
@@ -1072,13 +1082,14 @@ vcard_emul_options(const char *args)
             vreaderOpt->card_type = type;
             vreaderOpt->type_params =
                 copy_string(type_params, type_params_length);
-            count = count_tokens(args, ',', ')');
+            count = count_tokens(args, ',', ')') + 1;
             vreaderOpt->cert_count = count;
             vreaderOpt->cert_name = (char **)qemu_malloc(count*sizeof(char *));
             for (i = 0; i < count; i++) {
-                const char *cert = args + 1;
-                args = strpbrk(args + 1, ",)");
+                const char *cert = args;
+                args = strpbrk(args, ",)");
                 vreaderOpt->cert_name[i] = copy_string(cert, args - cert);
+                args = strip(args+1);
             }
             if (*args == ')') {
                 args++;
-- 
1.7.6

[Qemu-devel] [PATCHv3 2/4] libcacard: introduce NEXT_TOKEN macro

[Christophe Fergeau]

vcard_emul_options now has repetitive code to read the current
token and advance to the next. After the previous changes,
this repetitive code can be moved in a NEXT_TOKEN macro to
avoid having this code duplicated.

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vcard_emul_nss.c |   71 +++++++++++++++-----------------------------
 1 files changed, 24 insertions(+), 47 deletions(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 9271f58..2a20bd6 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -975,6 +975,26 @@ find_blank(const char *str)
 static VCardEmulOptions options;
 #define READER_STEP 4
 
+/* Expects "args" to be at the beginning of a token (ie right after the ','
+ * ending the previous token), and puts the next token start in "token",
+ * and its length in "token_length". "token" will not be nul-terminated.
+ * After calling the macro, "args" will be advanced to the beginning of
+ * the next token.
+ * This macro may call continue or break.
+ */
+#define NEXT_TOKEN(token) \
+            (token) = args; \
+            args = strpbrk(args, ",)"); \
+            if (*args == 0) { \
+                break; \
+            } \
+            if (*args == ')') { \
+                args++; \
+                continue; \
+            } \
+            (token##_length) = args - (token); \
+            args = strip(args+1);
+
 VCardEmulOptions *
 vcard_emul_options(const char *args)
 {
@@ -1010,58 +1030,15 @@ vcard_emul_options(const char *args)
             }
             args = strip(args+1);
 
-            name = args;
-            args = strpbrk(args, ",)");
-            if (*args == 0) {
-                break;
-            }
-            if (*args == ')') {
-                args++;
-                continue;
-            }
-            name_length = args - name;
-            args = strip(args+1);
-
-            vname = args;
-            args = strpbrk(args, ",)");
-            if (*args == 0) {
-                break;
-            }
-            if (*args == ')') {
-                args++;
-                continue;
-            }
-            vname_length = args - vname;
-            args = strip(args+1);
-
-            type_params = args;
-            args = strpbrk(args, ",)");
-            if (*args == 0) {
-                break;
-            }
-            if (*args == ')') {
-                args++;
-                continue;
-            }
-            type_params_length = args - type_params;
-            args = strip(args+1);
-
+            NEXT_TOKEN(name)
+            NEXT_TOKEN(vname)
+            NEXT_TOKEN(type_params)
             type_params_length = MIN(type_params_length, sizeof(type_str)-1);
             strncpy(type_str, type_params, type_params_length);
             type_str[type_params_length] = 0;
             type = vcard_emul_type_from_string(type_str);
 
-            type_params = args;
-            args = strpbrk(args, ",)");
-            if (*args == 0) {
-                break;
-            }
-            if (*args == ')') {
-                args++;
-                continue;
-            }
-            type_params_length = args - type_params;
-            args = strip(args+1);
+            NEXT_TOKEN(type_params)
 
             if (*args == 0) {
                 break;
-- 
1.7.6

[Qemu-devel] [PATCHv3 3/4] libcacard: replace copy_string with strndup

[Christophe Fergeau]

copy_string reimplements strndup, this commit removes it and
replaces all copy_string uses with strndup.

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Reviewed-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vcard_emul_nss.c |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 2a20bd6..de324ba 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -925,17 +925,6 @@ vcard_emul_replay_insertion_events(void)
 /*
  *  Silly little functions to help parsing our argument string
  */
-static char *
-copy_string(const char *str, int str_len)
-{
-    char *new_str;
-
-    new_str = qemu_malloc(str_len+1);
-    memcpy(new_str, str, str_len);
-    new_str[str_len] = 0;
-    return new_str;
-}
-
 static int
 count_tokens(const char *str, char token, char token_end)
 {
@@ -1054,18 +1043,18 @@ vcard_emul_options(const char *args)
             }
             opts->vreader = vreaderOpt;
             vreaderOpt = &vreaderOpt[opts->vreader_count];
-            vreaderOpt->name = copy_string(name, name_length);
-            vreaderOpt->vname = copy_string(vname, vname_length);
+            vreaderOpt->name = qemu_strndup(name, name_length);
+            vreaderOpt->vname = qemu_strndup(vname, vname_length);
             vreaderOpt->card_type = type;
             vreaderOpt->type_params =
-                copy_string(type_params, type_params_length);
+                qemu_strndup(type_params, type_params_length);
             count = count_tokens(args, ',', ')') + 1;
             vreaderOpt->cert_count = count;
             vreaderOpt->cert_name = (char **)qemu_malloc(count*sizeof(char *));
             for (i = 0; i < count; i++) {
                 const char *cert = args;
                 args = strpbrk(args, ",)");
-                vreaderOpt->cert_name[i] = copy_string(cert, args - cert);
+                vreaderOpt->cert_name[i] = qemu_strndup(cert, args - cert);
                 args = strip(args+1);
             }
             if (*args == ')') {
@@ -1092,7 +1081,7 @@ vcard_emul_options(const char *args)
             args = strip(args+10);
             params = args;
             args = find_blank(args);
-            opts->hw_type_params = copy_string(params, args-params);
+            opts->hw_type_params = qemu_strndup(params, args-params);
         /* db="/data/base/path" */
         } else if (strncmp(args, "db=", 3) == 0) {
             const char *db;
@@ -1103,7 +1092,7 @@ vcard_emul_options(const char *args)
             args++;
             db = args;
             args = strpbrk(args, "\"\n");
-            opts->nss_db = copy_string(db, args-db);
+            opts->nss_db = qemu_strndup(db, args-db);
             if (*args != 0) {
                 args++;
             }
-- 
1.7.6

[Qemu-devel] [PATCHv3 4/4] libcacard: don't leak vcard_emul_alloc_arrays mem

[Christophe Fergeau]

vcard_emul_mirror_card and vcard_emul_init use
vcard_emul_alloc_arrays to allocate memory for temporary arrays
which will contain elements that in the end will be used one by
one in cac_card_init. The arrays themselves are never stored
anywhere, they are only used as temporary containers. Hence
the memory that was allocated for these arrays should be freed
after use or they will be leaked.

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
---
 libcacard/vcard_emul_nss.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index de324ba..4fee471 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -476,6 +476,7 @@ vcard_emul_mirror_card(VReader *vreader)
     VCardKey **keys;
     PK11SlotInfo *slot;
     PRBool ret;
+    VCard *card;
 
     slot = vcard_emul_reader_get_slot(vreader);
     if (slot == NULL) {
@@ -535,7 +536,12 @@ vcard_emul_mirror_card(VReader *vreader)
     }
 
     /* now create the card */
-    return vcard_emul_make_card(vreader, certs, cert_len, keys, cert_count);
+    card = vcard_emul_make_card(vreader, certs, cert_len, keys, cert_count);
+    qemu_free(certs);
+    qemu_free(cert_len);
+    qemu_free(keys);
+
+    return card;
 }
 
 static VCardEmulType default_card_type = VCARD_EMUL_NONE;
@@ -820,6 +826,9 @@ vcard_emul_init(const VCardEmulOptions *options)
             vreader_free(vreader);
             has_readers = PR_TRUE;
         }
+        qemu_free(certs);
+        qemu_free(cert_len);
+        qemu_free(keys);
     }
 
     /* if we aren't suppose to use hw, skip looking up hardware tokens */
-- 
1.7.6

Re: [Qemu-devel] [PATCHv3 0/4] libcacard fixes

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 418 bytes --]

On Fri, Jul 22, 2011 at 01:33:26PM +0200, Christophe Fergeau wrote:
> This is a resend of my series of libcacard fixes, no change since last
> version apart from the added "Reviewed-by" tag.

Scratch that, I put the wrong patches in the series (which strongly
indicates that I should merge libcacard: fix soft=... parsing in
vcard_emul_options as a fifth patch in this set). I'll resend the actual
series.

Christophe

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]