| Message ID | 1308926260-11995-1-git-send-email-cfergeau@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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
Hey, Alon asked me to split the parsing patches to make the review easier, to here they are. Changes since v1: - split first patch into 3 patches, v1 2/2 is unchanged Christophe Fergeau (4): libcacard: s/strip(args++)/strip(args+1) libcacard: fix soft=... parsing in vcard_emul_options libcacard: introduce NEXT_TOKEN macro libcacard: replace copy_string with strndup libcacard/vcard_emul_nss.c | 113 +++++++++++++++++-------------------------- 1 files changed, 45 insertions(+), 68 deletions(-)
On Mon, Jun 27, 2011 at 05:27:35PM +0200, Christophe Fergeau wrote: > Hey, > > Alon asked me to split the parsing patches to make the review easier, to here > they are. Reviewed-by: Alon Levy <alevy@redhat.com> > > Changes since v1: > - split first patch into 3 patches, v1 2/2 is unchanged > > Christophe Fergeau (4): > libcacard: s/strip(args++)/strip(args+1) > libcacard: fix soft=... parsing in vcard_emul_options > libcacard: introduce NEXT_TOKEN macro > libcacard: replace copy_string with strndup > > libcacard/vcard_emul_nss.c | 113 +++++++++++++++++-------------------------- > 1 files changed, 45 insertions(+), 68 deletions(-) > > -- > 1.7.5.4 > >
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++;
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 <cfergeau@redhat.com> --- libcacard/vcard_emul_nss.c | 90 +++++++++++++++++++------------------------- 1 files changed, 39 insertions(+), 51 deletions(-)