lkml.org 
[lkml]   [2016]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: sscanf: implement basic character sets
+++ Rasmus Villemoes [23/02/16 23:47 +0100]:
>On Tue, Feb 23 2016, Jessica Yu <jeyu@redhat.com> wrote:
>
>> Implement basic character sets for the '%[]' conversion specifier.
>>
>>
>> lib/vsprintf.c | 41 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 41 insertions(+)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 525c8e1..983358a 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -2714,6 +2714,47 @@ 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;
>> +
>> + 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);
>> + /* no matches */
>> + if (!len) {
>> + kfree(set);
>> + return num;
>> + }
>> +
>> + while (len-- && field_width--)
>> + *s++ = *str++;
>> + *s = '\0';
>> + kfree(set);
>> + num++;
>> + }
>> + continue;
>> case 'o':
>> base = 8;
>> break;
>
>(1) How do we know that doing a memory allocation would be ok, and then
>with GFP_KERNEL? vsnprintf can be called from just about any context, so
>I don't think that would fly there. Sooner or later someone is going to
>be calling sscanf with a spinlock held, methinks.
>
>(2) I think a field width should be mandatory (so %[ should simply be
>regarded as malformed - it should be %*[ or %n[ for some explicit
>decimal n). That will allow the compiler or other static analyzers to do
>sanity checking, and we'll probably be saved from a few buffer
>overflows down the line.
>
>It's a bit sad that the C standard doesn't include the terminating '\0'
>in the field width, so one would sometimes have to write
>'(int)sizeof(buf)-1', but there's not much to do about that. On that
>note, it seems that your field width handling is off-by-one.
>
>To get rid of the allocation, why not use a small bitmap? Something like
>
>{
> char *s = (char *)va_arg(args, char *);
> DECLARE_BITMAP(map, 256) = {0};
> bool negate = false;
>
> /* a field width is required, and must provide room for at least a '\0' */
> if (field_width <= 0)
> return num;
>
> if (*fmt == '^') {
> negate = true;
> ++fmt;
> }
> for ( ; *fmt && *fmt != ']'; ++fmt)
> set_bit((u8)*fmt, map);
> if (!*fmt) // no ], so malformed input
> return num;
> ++fmt;
> if (negate) {
> bitmap_complement(map, map, 256);
> clear_bit(0, map); // this avoids testing *str != '\0' below
> }
>
> if (!test_bit((u8)*str, map)) // match must be non-empty
> return num;
> while (test_bit((u8)*str, map) && --field_width) {
> *s++ = *str++;
> }
> *s = '\0';
> ++num;
>}

I quite like this idea, as it avoids allocations and doesn't need
strcspn/strspn. What do other people think?

\
 
 \ /
  Last update: 2016-02-24 07:22    [W:0.065 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site