[PATCH] Fix string to integer conversion for occurences in source3

[Jeremy Allison]

On Fri, Jan 11, 2019 at 09:59:09AM +0100, swen via samba-technical wrote:
> > 
> > Some of the VFS returns depend on errno coming out
> > correctly from a call (yes I know, they're bad interfaces but
> > they are what they are). Normally this is done by
> > ensuring that the errno-setting function is the
> > last one called before exit from the VFS, but any
> > change that explicitly sets errno can cause subtle
> > hard to track bugs.
> Hmm, ok, but this is done already by calling the strtoul()
> ..is that taken into account ?

No, and that would be an example of an unintended
side-effect that we didn't consider. i.e. this is
a bug.

It doesn't happen in practice as this is a very
unlikely failure case, but it is possible to happen.

So it's an existing bug that never occurs.

> > So what I'd prefer is for you to create wrapper
> > functions that preserve an existing errno:
> I was thinking of a wrapper already but for different purpose (sign)
> and planned that one as a second step. That was maybe not too smart.
> 
> > 
> > strtoul_err()
> > 
> > and:
> > 
> > strtoull_err()
> > 
> > Here is a sample implementation (not compiled or tested I'm
> > afraid :-):
> > 
> > unsigned long int strtoul_err(const char *nptr, char **endptr, int
> > base, int *perr)
> > {
> > 	unsigned long int val;
> > 	int saved_errno = errno;
> > 
> > 	*perr = 0;
> > 	errno = 0;
> > 	val = strtoul(nptr, endptr, base);
> > 	if (errno != 0) {
> > 		*perr = errno;
> > 	}
> > 	errno = saved_errno;
> > 	return val;
> > }
> > 
> As briefly stated above I think the "issue" of a signed number in the
> string needs some handling. In addition, I think your suggested

Why ? The current code doesn't consider a leading minus
sign, doing so means a change in behavior which I want to
avoid.

> saved_errno handling is preventing the usage of the *real* errno as
> parameter if that feature isn't wanted/required.
> 
> Anyhow, so I would suggest the following as a wrapper... taking your
> one as the template.
> 
> unsigned long long strtoull_safe(const char *nptr, char **endptr, int base, int *err)
> {
> 	unsigned long long val;
> 	int saved_errno = errno;
> 
> 	errno = 0;
> 	val = strtoull(nptr, endptr, base);
> 	if (errno != 0) {
> 		if (err != NULL) {
> 			*err = errno;
> 			errno = saved_errno;
> 		}
> 		return 0;
> 	}
> 
> 	if (index(nptr, '-') != NULL) {
> 		/* EADV (Advertise Error) to flag the conversion
> 		 * of a string representing a negative number.
> 		 */
> 		errno = EADV;
> 		if (err != NULL) {
> 			*err = errno;
> 			errno = saved_errno;
> 		}
> 	}
> 
> 	return val;
> }
> 
> This version has the nice "side-effect / feature" that whoever doesn't need
> the saved_errno capability doesn't have to use it and ends up 
> with the standard behaviour. We could add a check if errno is still given

Actually I explicitly wanted the 'standard' behavior to
be impossible, as it leads to side effects that people
don't consider. Unexpected side effects cause bugs.

Make people consider the error properly. If you want the
ability to assign to errno you could change the function signatures
to be:

int strtoul_err(const char *nptr, char **endptr, int base, unsigned long int *pval)
int strtoull_err(const char *nptr, char **endptr, int base, unsigned long long int *pval)

and return the error (or zero) as the return parameter.

Actually scratch that - it is also a bad interface, as people will
easy misuse the assigned return.

Nope, I think *requiring* the 'int *perr' parameter to
be non-null with my original proposed functions is the
correct choice.

It's unusual for people to pass &errno as a parameter
for exactly these kind of reasons, so I'm OK with that
being an implicit restriction.

> as the addtl. last parameter...resulting in rubbish, just to be bullett-proof.
> In adition, the conversion of a "negative" string would be flagged and
> the caller can then decide if that is wanted or not. 

index() isn't a widely used function in Samba (this would
be the first use in source3) - I had to look it up. Plus
merely looking for '-' doesn't catch it if it's at the end
of the string, it only affects string conversion if it's
at the beginning.

I don't see why you want to change the return for -ve
numbers when the existing code paths don't consider
them. Let's just leave that alone until you can think
of an error caused by this. I can't think of one so
IMHO this is pure over-design.

> Even that could be slightly extended to verify whether the "-" appeared as part
> of the number of somewhere in the remaining string.
> Not sure if those extra checks would be really required/wanted.
> 
> > Put them in lib/uil/util.c
> > 
> > Then it should be easy to just call these instead of the horrid
> > strtoul() and strtoull() functions and know that they're safe
> > to use anywhere. Will mean less code changes, and just the
> > additional &err parameter being checked.
> You're absolutely right !
> > 
> > Does that sound like a plan ?
> It does !
> 
> So what do you think of the above version of the new string conversion
> function ?
> If that's ok with you, I'll create a patch getting it into the code.

Don't like it :-). See above.

I prefer the functions I posted I'm afraid.

Remember this is an almost paranoid code path fix
(based on people mostly misconfiguring smb.conf
params) so I don't want to bike-shed all day on
this.

Jeremy.