Re: [PATCH] strtoul_ui: reject negative values
From: Marc Branchaud <hidden>
Date: 2016-06-15 23:06:35
On 15-09-17 10:37 AM, Matthieu Moy wrote:
quoted hunk ↗ jump to hunk
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 <redacted> Signed-off-by: Matthieu Moy <redacted> --- 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.