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)
> 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
>
>
