From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751537AbbEDVsk (ORCPT ); Mon, 4 May 2015 17:48:40 -0400 Received: from mail-lb0-f169.google.com ([209.85.217.169]:35875 "EHLO mail-lb0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751167AbbEDVsb (ORCPT ); Mon, 4 May 2015 17:48:31 -0400 From: Rasmus Villemoes To: Alexey Dobriyan Cc: Andrew Morton , Linux Kernel Subject: Re: [PATCH 01/10] Add parse_integer() (replacement for simple_strto*()) Organization: D03 References: <20150502004714.GA21655@p183.telecom.by> <87h9rsiigj.fsf@rasmusvillemoes.dk> <87y4l4gumd.fsf@rasmusvillemoes.dk> <20150504195435.GA21686@p183.telecom.by> X-Hashcash: 1:20:150504:adobriyan@gmail.com::q+UtkU+AIqGHE6UE:0000000000000000000000000000000000000000000biz X-Hashcash: 1:20:150504:akpm@linux-foundation.org::IuI0GXZGK67yw+gB:0000000000000000000000000000000000001TW4 X-Hashcash: 1:20:150504:linux-kernel@vger.kernel.org::hz3IkCk5HvIty1Qd:0000000000000000000000000000000009TGc Date: Mon, 04 May 2015 23:48:27 +0200 In-Reply-To: <20150504195435.GA21686@p183.telecom.by> (Alexey Dobriyan's message of "Mon, 4 May 2015 22:54:35 +0300") Message-ID: <87383cui8k.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 04 2015, Alexey Dobriyan wrote: >> There are lots of callers of memparse(), and I don't think any of them >> are prepared to handle *endp ending up pointing before the passed-in >> string (-EINVAL == -22, -ERANGE == -34). I can easily see how that could >> lead to an infinite loop, maybe worse. > > Yeah, possible bug could become worse, I'll add error checking, > but, seriously, you're defending this :^) > > case Opt_nr_inodes: > ===> /* memparse() will accept a K/M/G without a digit */ > ===> if (!isdigit(*args[0].from)) > ===> goto bad_val; > pconfig->nr_inodes = memparse(args[0].from, &rest); > break; > No, I'm not defending memparse(), simple_strto* or any of their callers. I'm just trying to say that you can't change the semantics of memparse() without considering all its callers. I don't think there's any way to "add error checking" and preserve the exact memparse() semantic - in other words, I don't think simple_strto* can actually be implemented in terms of parse_integer. But that's not a bad thing - we want to get rid of those. > memparse() is misdesigned in the same sense strtoul() is misdesigned. > Every "memparse(s, NULL)" user is a bug for example. Yes, memparse is misdesigned, since it doesn't have a way to indicate error. That leads me to: There's no point in adding error checking to the integer parsing part without also checking the left shifts for overflow. I think the right approach is to rename memparse to legacy_memparse and introduce a memparse with semantics that allow error checking. One could start by introducing that under the name sane_memparse. But there are probably lots of simple_strto*() uses that are easier to eliminate. Rasmus