Re: [PATCH] strtoul_ui: reject negative values
- From
Marc Branchaud <marcnarc@xiplink.com>
- Date
- Sep 17, 2015, 15:17 UTC
- Message-ID
- <55FAD981.5080808@xiplink.com>
- In-Reply-To
- <1442500646-15293-1-git-send-email-Matthieu.Moy@imag.fr>
On 15-09-17 10:37 AM, Matthieu Moy wrote:
Show 35 quoted lines
> strtoul_ui uses strtoul to get a long unsigned, then checks that casting
> to unsigned does not lose information and return the casted value.
>
> On 64 bits architecture, checking that the cast does not change the value
> catches most errors, but when sizeof(int) == sizeof(long) (e.g. i386),
> the check does nothing. Unfortunately, strtoul silently accepts negative
> values, and as a result strtoul_ui("-1", ...) raised no error.
>
> This patch catches negative values before it's too late, i.e. before
> calling strtoul. We still silently accept very large integers that wrap
> to a valid "unsigned int".
>
> Reported-by: Max Kirillov <max@max630.net>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> So, here's a proper patch (I mean, a band-aid patch, but properly
> send ;-) ).
>
> It should be merged before Kartik's series (or inserted at the start
> of the series) so that we get the fix before the test breakage.
>
> git-compat-util.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f649e81..1df82fa 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)
> char *p;
>
> errno = 0;
> + /* negative values would be accepted by strtoul */
> + if (strchr(s, '-'))
> + return -1;I think this is broken, in that it doesn't match strtoul's normal behaviour, for strings like "1234-5678", no?
The test also doesn't work if the string has leading whitespace (" -5").> ul = strtoul(s, &p, base); > if (errno || *p || p == s || (unsigned int) ul != ul) > return -1;
Hmm, but we check *p here, so IIUC it's an error if the string has any trailing non-digits. Weird.
M.