[PATCH] Fix string to integer conversion

[Ralph Böhme]

Hi Swen,

On Tue, Feb 26, 2019 at 02:24:07PM +0100, swen wrote:
>thanks for going through all this effort but ... I have a few comments

I was afraid of that... :)))

>On Tue, 2019-02-26 at 13:55 +0100, Ralph Böhme wrote:
>> > Yeah, I might be willing to cross that bridge. Let me go through
>> > the
>> > changes in the caller. Stay tuned...
>>
>> here's a branch with a few fixups:
>>
>> <
>> https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/strto_err
>> >
>>
>> We also need a doxygen comment on the new functions that explain
>> possible
>> results and errors.
>Ok, I will do that..
>
>>
>> For now I think we should just go with the integeroverflow and signed
>> detection.
>Why not adding the last one detecting the no-conversion patch ?
>
>Now a few comments regarding the FIXUPS.
>
>general:
>Why do split all initializations at declaration time into two hunks ?
>I don't see a reason for that.

Looks better. :) I guess there's no hard rule for that, in boilerplace code 
around tevent_req it makes sense to combine variable declaration and function 
call. But it places where it's not boilerplate stuff, it's better to seperate it 
for readability.

>diff --git a/source3/lib/messages_dgm.c b/source3/lib/messages_dgm.c
>index 4836697..b233c68 100644 (file)
>--- a/source3/lib/messages_dgm.c
>+++ b/source3/lib/messages_dgm.c
>@@ -1454,6 +1454,9 @@ static int messaging_dgm_read_unique(int fd,
>uint64_t *punique)
>        buf[rw_ret] = '\0';
>
>        unique = strtoull_err(buf, &endptr, 10, &error);
>+       if ((unique == 0) && (errno == EINVAL)) {
>+               return EINVAL;
>+       }
>        if (error != 0) {
>                return error;
>        }
>This is not needed. Exactly the same happens without this addition.

Oh, sorry, forgot to explain that. The idea is that this patchset should only 
change the callers to rely on the new functionality that integer overflow is 
detected and keep all other checks in the callers.

Imho the above hung is indeed not correct, but then we should make that a 
seperate change.

>diff --git a/source3/utils/pdbedit.c b/source3/utils/pdbedit.c
>index 51db671..c80d541 100644 (file)
>--- a/source3/utils/pdbedit.c
>+++ b/source3/utils/pdbedit.c
>@@ -599,14 +599,13 @@ static int set_user_info(const char *username,
>const char *fullname,
>
>                if (strcmp(kickoff_time, "never") != 0) {
>                        int error = 0;
>-                       uint32_t num = strtoul_err(kickoff_time,
>-                                                  &endptr,
>-                                                  10,
>-                                                  &error);
>+                       uint32_t num;
>
>+                       num = strtoul_err(kickoff_time, &endptr, 10,
>&error);
>                        if ((endptr == kickoff_time) ||
>                            (endptr[0] != '\0') ||
>-                           (error != 0)) {
>+                           (error != 0))
>+                       {
>                                fprintf(stderr, "Failed to parse
>kickoff time\n");
>                                return -1;
>                        }
>I'm pretty sure our coding guide says somehting else.

README.Coding:

If the beginning statement has to be broken across lines due to length,
the beginning brace should be on a line of its own.

>Ok, I guess there's no poijhnt in commenting all of them.
>At the end of the day, if that makes you feel better about it,
>I won't object.

Yes, feels better. :)

>How do we proceed from here ?
>I have the full patchset ready (incl. the no-conversion patch) but
>without your fixups.
>Since you have it all together, I guess there's no need for me to send
>it again, right ?

If you're happy with the fixups, just fixup' em.

>So please let me know what's next.

1. commit messages
2. doxygen comments
3. wait for Jeremy :)

>Thanks for your support in advance, really appreciated !!!

Happy to help!

-slow

-- 
Ralph Boehme, Samba Team                https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
GPG-Fingerprint   FAE2C6088A24252051C559E4AA1E9B7126399E46