From: Phillip Wood Date: Tue, 25 Oct 2022 10:00:02 GMT Subject: Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow Message-ID: <190de302-0898-50a1-1a9d-ab1ccc5c6bb8@dunelm.org.uk> In-Reply-To: On 23/10/2022 06:57, René Scharfe wrote: > Am 22.10.22 um 18:51 schrieb Junio C Hamano: >> René Scharfe writes: >> >>>>> + if (max < 0) >>>>> + BUG("max must be a positive integer"); >>>> >>>> In parse_signed(), would we expect to accept end-user input that is >>>> a negative integer? We must. Otherwise we would not be calling a >>>> "signed" parser. Now, are there cases where the valid value range >>>> is bounded by a negative integer at the top? No current callers may >>>> pass such a value, but is it reasonable to add such a new constraints >>>> to an existing API function? >>> >>> Hmm, if minimum and maximum are not symmetric, then we need to supply >>> both, don't we? >> >> Ah, thanks for injecting doze of sanity---I totally missed that the >> bound was about the absolute value, so we can say "this is signed, >> and the allowed values are (-3, -2, -1, 0, 1, 2, 3). If so, then the >> "reject negative max" in the posted patch is not a problem as I said >> above. I somehow thought that giving -1 as "max" would allow callers >> to say "non-negative numbers are not allowed". But that is not what >> is going on. > > Right, currently the value of `max` is used to check the absolute value, > i.e. it imposes a limit in both the positive and negative direction. > >> Allowing callers to specify both lower and uppoer bounds so that >> they can say "the allowed values are (-1, 0, 1, 2, 3)", while it >> might make it more useful, is a separate new feature development and >> outside the scope of "let's tighten the parsing of end user input" >> Phillip has here. > > Allowing arbitrary limits in both directions might be a new feature, but > it's required if we want to support the full range of values. E.g. on > my system INT_MAX is 2147483647 and INT_MIN is -2147483647-1. Currently > git_parse_int() rejects INT_MIN as out of range. > > In my eyes the assumption that a single limit can be used to check both > directions of the signed number line is flawed and caused the undefined > behavior. Dropping it avoids tricky calculations that try to infer the > lower limit somehow and opens the full range of values to us. > > That said, I'm not sure how useful the values INT_MIN, INT64_MIN and > SSIZE_MIN (which unlike SSIZE_MAX is not defined by POSIX [*]) actually > are. But doing the checks properly requires separate min and max values. I'm happy to go either way, while I agree passing separate limits to allow INT_MIN is technically correct I'm not sure anyone has complained that the current code is too restrictive. Best Wishes Phillip > René > > > [*] Perhaps git_parse_ssize_t() should reject values less than -1; only > that single negative number is needed to represent errors or unlimited.