From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1756373AbcBXFjz (ORCPT <rfc822;w@1wt.eu>);
	Wed, 24 Feb 2016 00:39:55 -0500
Received: from mx1.redhat.com ([209.132.183.28]:37151 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750779AbcBXFjy (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 24 Feb 2016 00:39:54 -0500
Date: Wed, 24 Feb 2016 00:39:51 -0500
From: Jessica Yu <jeyu@redhat.com>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Kees Cook <keescook@chromium.org>, linux-kernel@vger.kernel.org
Subject: Re: sscanf: implement basic character sets
Message-ID: <20160224053951.GE30919@packer-debian-8-amd64.digitalocean.com>
References: <1456259902-31792-1-git-send-email-jeyu@redhat.com>
 <87bn77gi34.fsf@rasmusvillemoes.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <87bn77gi34.fsf@rasmusvillemoes.dk>
X-OS: Linux eisen.io 3.16.0-4-amd64 x86_64
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

+++ 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?