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

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

[Christophe Fergeau]

Hi,

This is a resend of my series of libcacard fixes, with the correct patches this
time.

Changes since v3:
- the series now includes the intended patches

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] [PATCHv4 1/4] libcacard: s/strip(args++)/strip(args+1)

[Christophe Fergeau]

vcard_emul_options used args = strip(args++) a few times, which
was not returning the expected result since the rest of the code
expected args to be increased by at least 1, which is not the case
if *args is not a blank space when this function is called.
Replace these calls by "strip(args+1)" which will do what we expect.

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

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index f3db657..184252f 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1041,7 +1041,7 @@ vcard_emul_options(const char *args)
                 args++;
                 continue;
             }
-            args = strip(args++);
+            args = strip(args+1);
             type_params = args;
             args = strpbrk(args + 1, ",)");
             if (*args == 0) {
@@ -1052,7 +1052,7 @@ vcard_emul_options(const char *args)
                 continue;
             }
             type_params_length = args - name;
-            args = strip(args++);
+            args = strip(args+1);
             if (*args == 0) {
                 break;
             }
-- 
1.7.6

[Qemu-devel] [PATCHv4 2/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] [PATCHv4 3/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] [PATCHv4 4/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

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

[Alon Levy]

On Fri, Jul 22, 2011 at 01:42:16PM +0200, Christophe Fergeau wrote:
> Hi,
> 

I'll add all of these to the pull request, thanks.


> This is a resend of my series of libcacard fixes, with the correct patches this
> time.
> 
> Changes since v3:
> - the series now includes the intended patches
> 
> 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
> 
>