From mboxrd@z Thu Jan 1 00:00:00 1970 From: jamie.couture at gmail.com (Jamie Couture) Date: Wed, 13 May 2015 10:57:00 -0400 Subject: [PATCH] cgit: use strtol_i instead of atoi In-Reply-To: <20150513134556.GC10518@serenity.lan> References: <1431523261-982-1-git-send-email-ncopa@alpinelinux.org> <20150513134124.GB10518@serenity.lan> <20150513134556.GC10518@serenity.lan> Message-ID: <20150513145700.GA7679@neptune> On Wed, May 13, 2015 at 02:45:56PM +0100, John Keeping wrote: > On Wed, May 13, 2015 at 02:41:24PM +0100, John Keeping wrote: > > On Wed, May 13, 2015 at 03:35:29PM +0200, Jason A. Donenfeld wrote: > > > Anybody have any objections to this? In some cases it's slightly more > > > verbose, but otherwise, I can't see any downsides. > > > > It's worse if there is trailing data. Since there's nothing obvious we > > can do if the input is bad, I'm not sure how much we care (i.e. ignoring > > the return value from strtol_i is OK) but whereas atoi will parse a > > valid value followed by trailing garbage strtol_i will just fail. > > > > Worse than that, if it fails it leaves the result uninitialized, which > > doesn't matter in the cases where we just update a variable, but at > > least one part of this patch introduces a new variable that is not set > > if strtol_i fails. > > Oops... I didn't double-check the patch before sending, it does always > initialize the variables first so the only worry is trailing garbage. > > Of course, if atoi leads to SQL injection, what makes strtol safe? The > test seems fundamentally useless; AFAICT the whole point is that if I > parse some user input and use without validating it then I can end up > doing something like: > > int num_items = ; > items = malloc(num_items * sizeof(*items)); > > leading to integer overflow. But the mechanism used to convert the user > input from a string to an integer is completely irrelevant. Exactly. I'm not familiar with the utility, but can nessus have exceptions to a rule? In cgit's case, I don't see the use of atoi() as a problem in this context.