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

[Alon Levy <alevy@redhat.com>]

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 <cfergeau@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 **)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.5.4
> 
>