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

[swen]

On Thu, 2019-01-10 at 12:09 -0800, Jeremy Allison wrote:
> On Thu, Jan 10, 2019 at 06:54:51PM +0100, swen via samba-technical
> wrote:
> > This is patchset is the successor of the mail thread
> > 	[PATCH] [tldap] check for successful string conversion
> > which I had with Volker today.
> > 
> > As stated in the subject this patch-set updates all occurences of 
> > the usage strtoul(l) to verify the result is valid.
> > 
> > Please review and push if happy.
> > 
> > Thanks for your support in advance.
Hi Jeremy,

thanks for your quick and comprehensive response,
now I'm confident again that we'll find a solution
for the technical issue.

> 
> Hi Swen,
> 
> I followed the conversation with Volker on the strtoul(l)
> saga, and in the epic words of Futurama, you are "Technically
> Correct, The Best Kind of Correct" :-).
> 
> https://www.youtube.com/watch?v=hou0lU8WMgo

That's what I'm saying :-)

> 
> Having said that there are a bunch of issues with
> this change that we need to fix.
> 
> 1). nttime_from_string() is actually an unused function.
> 
> diff --git a/source3/lib/time.c b/source3/lib/time.c
> index 30ad1ec9a01..09c5b0656f6 100644
> --- a/source3/lib/time.c
> +++ b/source3/lib/time.c
> @@ -42,6 +42,7 @@
>  */
>  NTTIME nttime_from_string(const char *s)
>  {
> +     errno = 0;
>       return strtoull(s, NULL, 0);
>  }
> 
> Rather than fix, just delete it !
ok, done (in seperate patch).

> 
> 2). Setting errno=0 inside VFS code is fraught with
> danger. Your changes indirectly do this by changing
> conv_str_size() to set errno to zero as a side effect
> which is called by some of the VFS functions.
> 
> 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 ?
> 
> 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
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
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. 
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.

Thanks for your support in advance.

Cheers Swen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190111/b398c58f/signature.sig>