[PATCH] Fix string to integer conversion

Ralph Böhme slow at samba.org
Tue Feb 26 10:15:05 UTC 2019


On Tue, Feb 26, 2019 at 09:22:07AM +0100, swen wrote:
>Hi Ralph
>
>I just wish you would have come up with your thoughts/ideas
>when the discussion was on 3-4 weeks ago :-)

yeah, sorry, been really busy so I wasn't able to keep up with some stuff.

>On Mon, 2019-02-25 at 18:56 +0100, Ralph Böhme via samba-technical
>wrote:
>> Hi Swen,
>>
>> On Mon, Feb 25, 2019 at 01:00:34PM +0000, Swen Schillig wrote:
>>
>> The problem is that the new function and the use of the new function
>> in a bunch
>> of places suggests that you can *rely* on the error out variable
>> being for
>> strings like "" or "foo".
>>
>> Eg in one place your patch adds this:
>>
>> --- a/ctdb/tools/ctdb.c
>> +++ b/ctdb/tools/ctdb.c
>> @@ -544,7 +545,10 @@ static bool db_exists(TALLOC_CTX *mem_ctx,
>> struct ctdb_context *ctdb,
>>         }
>>
>> -               id = strtoul(db_arg, NULL, 0);
>> +               id = strtoul_err(db_arg, NULL, 0, &ret);
>> +               if (ret != 0) {
>> +                       return false;
>> +               }
>>
>What about if something like this is added ?
>
>--- a/lib/util/util.c
>+++ b/lib/util/util.c
>@@ -64,6 +64,13 @@ strtoul_err(const char *nptr, char **endptr, int base, int *err)
>                *endptr = tmp_endptr;
>        }
>
>+       if (nptr == tmp_endptr) {
>+               /* we got an invalid number-string resulting in no conversion */
>+               *err = EINVAL;
>+               errno = save_errno;
>+               return val;
>+       }
>+
>        if (errno != 0) {
>                *err = errno;
>                errno = saved_errno;
>
>This will catch the non-conversion case and leaves everything else as it is ?

Then I guess we also need

+       if (nptr[0] == '\0') {
+               /* we got an empty string */
+               *err = EINVAL;
+               errno = save_errno;
+               return val;
+       }
+

before the call to strtoul().

Then lets remove the out-parameter uglyness and we're going somewhere:

bool tstrtoull(const char *nptr, char **endptr, int base, unsigned long long int *val);

:)

Let's see what Jeremy has to say on it, I'll give him a chance to convince me 
that strtoull_err() in the current form is really a good idea.

I imagine one previous argument has been, that a strict implementation of a new 
conversion function, be it strtoull_err() or tstrtoull(), may not be suitable to 
replace the existing call to strtoull() in all places due to the behaviour 
change?

Btw, there'a bug in strtoull_err(), fixup attached.

-slow

-- 
Ralph Boehme, Samba Team                https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
GPG-Fingerprint   FAE2C6088A24252051C559E4AA1E9B7126399E46
-------------- next part --------------
From 30162f931668894e883d4f125fb34cb33f86e3c7 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 26 Feb 2019 10:55:03 +0100
Subject: [PATCH] FIXUP

---
 lib/util/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/util/util.c b/lib/util/util.c
index 428bad7d285..ce91e8aa44b 100644
--- a/lib/util/util.c
+++ b/lib/util/util.c
@@ -91,7 +91,7 @@ strtoull_err(const char *nptr, char **endptr, int base, int *err)
 	errno = 0;
 	*err = 0;
 
-	val = strtoull(nptr, endptr, base);
+	val = strtoull(nptr, &tmp_endptr, base);
 
 	if (endptr != NULL) {
 		*endptr = tmp_endptr;
-- 
2.17.2



More information about the samba-technical mailing list