Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1340532 > unrolled thread

Re: [PATCH v2] sscanf: implement basic character sets

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] sscanf: implement basic character sets Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-02-23 12:00 +0100
    Re: [PATCH v2] sscanf: implement basic character sets Kees Cook <keescook@chromium.org> - 2016-02-23 20:10 +0100
      Re: sscanf: implement basic character sets Jessica Yu <jeyu@redhat.com> - 2016-02-23 20:50 +0100
    Re: sscanf: implement basic character sets Jessica Yu <jeyu@redhat.com> - 2016-02-23 20:30 +0100

#1340532 — Re: [PATCH v2] sscanf: implement basic character sets

On Mon, 2016-02-22 at 16:24 -0500, Jessica Yu wrote:
> Implement basic character sets for the '%[]' conversion specifier.
> 
> The '%[]' conversion specifier matches a nonempty sequence of
> characters
> from the specified set of accepted (or with '^', rejected) characters
> between the brackets. The substring matched is to be made up of
> characters
> in (or not in) the set. This implementation differs from its glibc
> counterpart in that it does not support character ranges (e.g., 'a-z' 
> or
> '0-9'), the hyphen '-' is *not* a special character, and the brackets
> themselves cannot be matched.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
> Patch based on linux-next-20160222.
> 
> v2:
>  - Use kstrndup() to copy the character set from fmt instead of using
> a
>    statically allocated array
>  
>  lib/vsprintf.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 525c8e1..93a6f52 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2714,6 +2714,45 @@ int vsscanf(const char *buf, const char *fmt,
> va_list args)
>  			num++;
>  		}
>  		continue;
> +		case '[':
> +		{
> +			char *s = (char *)va_arg(args, char *);
> +			char *set;
> +			size_t (*op)(const char *str, const char
> *set);
> +			size_t len = 0;
> +			bool negate = (*(fmt) == '^');
> +
> +			if (field_width == -1)
> +				field_width = SHRT_MAX;

I'm not sure if it's needed here. It will count down till 0 in any
case.

> +
> +			op = negate ? &strcspn : &strspn;
> +			if (negate)
> +				fmt++;

> +
> +			len = strcspn(fmt, "]");
> +			/* invalid format; stop here */
> +			if (!len)
> +				return num;
> +
> +			set = kstrndup(fmt, len, GFP_KERNEL);
> +			if (!set)
> +				return num;
> +
> +			/* advance fmt past ']' */
> +			fmt += len + 1;
> +
> +			len = (*op)(str, set);

Can we use just normal form:
 op();
?

> +			/* no matches */
> +			if (!len)

Memory leak here.

> +				return num;
> +
> +			while (*str && len-- && field_width--)
> +				*s++ = *str++;

Looks like strcpy() variant. First of all, is it possible to have *str
== '\0' when len != 0?

> +			*s = '\0';
> +			kfree(set);
> +			num++;
> +		}
> +		continue;
>  		case 'o':
>  			base = 8;
>  			break;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [next] | [standalone]

#1340953

On Tue, Feb 23, 2016 at 2:56 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2016-02-22 at 16:24 -0500, Jessica Yu wrote:
>> Implement basic character sets for the '%[]' conversion specifier.

What part of the kernel will be using this feature, by the way?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1340974 — Re: sscanf: implement basic character sets

+++ Kees Cook [23/02/16 11:00 -0800]:
>On Tue, Feb 23, 2016 at 2:56 AM, Andy Shevchenko
><andriy.shevchenko@linux.intel.com> wrote:
>> On Mon, 2016-02-22 at 16:24 -0500, Jessica Yu wrote:
>>> Implement basic character sets for the '%[]' conversion specifier.
>
>What part of the kernel will be using this feature, by the way?
>

I explained the motivation a bit more in patch v1's cover letter:
https://lkml.kernel.org/g/1455931259-27117-1-git-send-email-jeyu@redhat.com

The original idea stemmed from a discussion from the kernel livepatch mailing list:
https://lkml.org/lkml/2016/2/8/790

We were looking for a way to parse out substrings delimited by
something other than spaces. Specifically, in livepatch we are parsing
symbol names that contain substrings (which contain livepatch-specific
information) delimited by '.' and ','. Instead of manually looking for
these delimiters and adding a lot of string code to livepatch, it
would be cleaner to have a single sscanf() call to do the parsing for us.

Jessica

[toc] | [prev] | [next] | [standalone]

#1340964 — Re: sscanf: implement basic character sets

+++ Andy Shevchenko [23/02/16 12:56 +0200]:
>On Mon, 2016-02-22 at 16:24 -0500, Jessica Yu wrote:
>> Implement basic character sets for the '%[]' conversion specifier.
>>
>> The '%[]' conversion specifier matches a nonempty sequence of
>> characters
>> from the specified set of accepted (or with '^', rejected) characters
>> between the brackets. The substring matched is to be made up of
>> characters
>> in (or not in) the set. This implementation differs from its glibc
>> counterpart in that it does not support character ranges (e.g., 'a-z'
>> or
>> '0-9'), the hyphen '-' is *not* a special character, and the brackets
>> themselves cannot be matched.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>> Patch based on linux-next-20160222.
>>
>> v2:
>>  - Use kstrndup() to copy the character set from fmt instead of using
>> a
>>    statically allocated array
>>  
>>  lib/vsprintf.c | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 525c8e1..93a6f52 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -2714,6 +2714,45 @@ int vsscanf(const char *buf, const char *fmt,
>> va_list args)
>>  			num++;
>>  		}
>>  		continue;
>> +		case '[':
>> +		{
>> +			char *s = (char *)va_arg(args, char *);
>> +			char *set;
>> +			size_t (*op)(const char *str, const char
>> *set);
>> +			size_t len = 0;
>> +			bool negate = (*(fmt) == '^');
>> +
>> +			if (field_width == -1)
>> +				field_width = SHRT_MAX;
>
>I'm not sure if it's needed here. It will count down till 0 in any
>case.

I think it might be good to be consistent with the '%s' specifier code
and have some sort of upper bound set, even if it is much more likely
that len will get to 0 before field_width does.

>> +
>> +			op = negate ? &strcspn : &strspn;
>> +			if (negate)
>> +				fmt++;
>
>> +
>> +			len = strcspn(fmt, "]");
>> +			/* invalid format; stop here */
>> +			if (!len)
>> +				return num;
>> +
>> +			set = kstrndup(fmt, len, GFP_KERNEL);
>> +			if (!set)
>> +				return num;
>> +
>> +			/* advance fmt past ']' */
>> +			fmt += len + 1;
>> +
>> +			len = (*op)(str, set);
>
>Can we use just normal form:
> op();
>?
>
>> +			/* no matches */
>> +			if (!len)
>
>Memory leak here.
>
>> +				return num;
>> +
>> +			while (*str && len-- && field_width--)
>> +				*s++ = *str++;
>
>Looks like strcpy() variant. First of all, is it possible to have *str
>== '\0' when len != 0?

Good point. The *str check is redundant, since after the call to
strspn/strcspn we know there are at least len bytes in str, so that
check can be removed.

>> +			*s = '\0';
>> +			kfree(set);
>> +			num++;
>> +		}
>> +		continue;
>>  		case 'o':
>>  			base = 8;
>>  			break;
>
>-- 
>Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>Intel Finland Oy
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web