From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758028AbcBXFNx (ORCPT ); Wed, 24 Feb 2016 00:13:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58833 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757092AbcBXFNu (ORCPT ); Wed, 24 Feb 2016 00:13:50 -0500 Date: Wed, 24 Feb 2016 00:13:47 -0500 From: Jessica Yu To: Andrew Morton Cc: Rasmus Villemoes , Andy Shevchenko , Kees Cook , linux-kernel@vger.kernel.org Subject: Re: sscanf: implement basic character sets Message-ID: <20160224051346.GD30919@packer-debian-8-amd64.digitalocean.com> References: <1456259902-31792-1-git-send-email-jeyu@redhat.com> <20160223140539.30a24c55c91a01ec51fd42ba@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20160223140539.30a24c55c91a01ec51fd42ba@linux-foundation.org> 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: X-Mailing-List: linux-kernel@vger.kernel.org +++ Andrew Morton [23/02/16 14:05 -0800]: >On Tue, 23 Feb 2016 15:38:22 -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 >> --- >> >> This patch adds support for the '%[' conversion specifier for sscanf(). >> This is useful in cases where we'd like to match substrings delimited by >> something other than spaces. The original motivation for this patch >> actually came from a livepatch discussion (See: https://lkml.org/lkml/2016/2/8/790), >> where we were trying to come up with a clean way to parse symbol names with >> substrings delimited by periods and commas. > > It would be better to include the justification right here in the > changelog please. > Not via some link-to-discussion and definitely not > below the ^--- marker! It's very important. Thanks for the corrections Andrew. I am however slightly confused, are you suggesting that I should provide a much more thorough explanation about the motivation here in the changelog (below the ^--- marker), or would this be better suited for a (separate) cover letter? >The deviation from the glibc behaviour is a bit of a worry, >particularly as it is done in a non-back-compat manner: code which >assumes "-" is non-magic might break if someone later adds range >support. > >Presumably we can live with that - there won't be many callsites and >they can be grepped for. But please, let's get a description of all >these considerations into the code as a comment. Probably it would be >helpful to include a little usage example in that comment. Hm, that is a very good point. At the moment we can be sure there aren't any users of sscanf() using the %[ conversion specifier, as it doesn't exist yet :-) But yes, this behavior should be documented clearly in a comment, so future users will be aware.. >> --- 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); > >Embedding a GFP_KERNEL allocation into vsscanf is problematic - it >limits the situations in which this functionality can be used. > >afaict the allocation is there merely so we can null-terminate the >string so we can use existing library functions (strcspn, strspn). Is >that compromise really worth it? We could pretty easily convert >strcspn() into > > strcnspn(const char *s, const char *reject, size_t len) > >and convert strcspn() to call that (ifndef __HAVE_ARCH_STRCSPN) > >In fact I think we could still use strspn() and strcspn() on `fmt' >directly? We just need to check for the return value exceeding `len' >and if so, treat that as a no-match? > Perhaps we can use Rasmus' bitmap solution, as it avoids the allocation altogether and it doesn't need to use strspn()/strcspn(). >> + 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; >