Re: [Qemu-devel] [PATCH 1/2] libcacard: fix soft=... parsing in vcard_emul_options

2011-06-27 Thread Christophe Fergeau 
On Fri, Jun 24, 2011 at 06:51:51PM +0200, Alon Levy wrote:
 On Fri, Jun 24, 2011 at 04:37:39PM +0200, Christophe Fergeau wrote:
  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,
  and there were some args = strip(args++); which also didn't do what
  was expected.
 
 Aren't there token parsing functions in libc that can be used if we
 want to fix the repetitiveness?

The only token parsing function I know of in libc is strtok{_r} which
modifies the input string, so it's not very well suited here. I'm not a
libc specialist though, so there might be some functions in libc (or qemu
helpers) that I don't know of. 

Christophe


pgpZkrVNT3zIm.pgp
Description: PGP signature

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

2011-06-24 Thread 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,
and there were some args = strip(args++); which also didn't do what
was expected.

Since the token parsing is always the same, I factored all the
repetitive code in a NEXT_TOKEN macro.

Signed-off-by: Christophe Fergeau cferg...@redhat.com
---
 libcacard/vcard_emul_nss.c |   90 +++-
 1 files changed, 39 insertions(+), 51 deletions(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index f3db657..2a20bd6 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -975,13 +975,31 @@ 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)
 {
 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,63 +1014,32 @@ 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;
 }
-name = args;
-args = strpbrk(args + 1, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-args = strip(args+1);
-name_length = args - name - 2;
-vname = args;
-args = strpbrk(args + 1, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-vname_length = args - name - 2;
 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;
+
+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);
-args = strpbrk(args, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-args = strip(args++);
-type_params = args;
-args = strpbrk(args + 1, ,));
-if (*args == 0) {
-break;
-}
-if (*args == ')') {
-args++;
-continue;
-}
-type_params_length = args - name;
-args = strip(args++);
+
+NEXT_TOKEN(type_params)
+
 if (*args == 0) {
 break;
 }
@@ -1072,13 +1059,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

Re: [Qemu-devel] [PATCH 1/2] libcacard: fix soft=... parsing in vcard_emul_options

2011-06-24 Thread Alon Levy 
On Fri, Jun 24, 2011 at 04:37:39PM +0200, Christophe Fergeau wrote:
 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,
 and there were some args = strip(args++); which also didn't do what
 was expected.

Aren't there token parsing functions in libc that can be used if we
want to fix the repetitiveness?

 
 Since the token parsing is always the same, I factored all the
 repetitive code in a NEXT_TOKEN macro.
 
 Signed-off-by: Christophe Fergeau cferg...@redhat.com
 ---
  libcacard/vcard_emul_nss.c |   90 
 +++-
  1 files changed, 39 insertions(+), 51 deletions(-)
 
 diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
 index f3db657..2a20bd6 100644
 --- a/libcacard/vcard_emul_nss.c
 +++ b/libcacard/vcard_emul_nss.c
 @@ -975,13 +975,31 @@ 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)
  {
  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,63 +1014,32 @@ 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;
  }
 -name = args;
 -args = strpbrk(args + 1, ,));
 -if (*args == 0) {
 -break;
 -}
 -if (*args == ')') {
 -args++;
 -continue;
 -}
 -args = strip(args+1);
 -name_length = args - name - 2;
 -vname = args;
 -args = strpbrk(args + 1, ,));
 -if (*args == 0) {
 -break;
 -}
 -if (*args == ')') {
 -args++;
 -continue;
 -}
 -vname_length = args - name - 2;
  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;
 +
 +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);
 -args = strpbrk(args, ,));
 -if (*args == 0) {
 -break;
 -}
 -if (*args == ')') {
 -args++;
 -continue;
 -}
 -args = strip(args++);
 -type_params = args;
 -args = strpbrk(args + 1, ,));
 -if (*args == 0) {
 -break;
 -}
 -if (*args == ')') {
 -args++;
 -continue;
 -}
 -type_params_length = args - name;
 -args = strip(args++);
 +
 +NEXT_TOKEN(type_params)
 +
  if (*args == 0) {
  break;
  }
 @@ -1072,13 +1059,14 @@ vcard_emul_options(const char *args)