Re: [PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i()

[Phillip Wood <phillip.wood123@gmail.com>]

On 14/10/2024 11:53, Patrick Steinhardt wrote:
> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>>
>>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>>>
>>> Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
>>> and strtol_i() for signed integers across multiple files. This change
>>> improves error handling and prevents potential integer overflow issues.
>>>
>>> The following files were updated:
>>> - daemon.c: Update parsing of --timeout, --init-timeout, and
>>>    --max-connections
>>> - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
>>>    tags
>>> - merge-ll.c: Enhance parsing of marker size in ll_merge and
>>>    ll_merge_marker_size
> 
> To me it's always an indicator that something should be split up across
> multiple commits once you have a bulleted list of changes in your commit
> message.

Agreed, but I think in this case there is a common theme (converting 
atoi() to a safer alternative) and the problem is with the commit 
message listing which files have changed rather than unrelated code 
changes being grouped together. This patch could be split up and if 
there were many more atoi() conversions it would need to be split to 
prevent it being too long but I don't think its essential to do so.

Best Wishes

Phillip

>>> This change allows for better error detection when parsing integer
>>> values from command-line arguments and IMAP responses, making the code
>>> more robust and secure.
>>>
>>> This is a #leftoverbit discussed here:
>>>   https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/
>>>
>>> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
>>>
>>> Cc: gitster@pobox.com
>>> Cc: Patrick Steinhardt <ps@pks.im>
>>> Cc: phillip.wood123@gmail.com
>>> Cc: Christian Couder <christian.couder@gmail.com>
>>> Cc: Eric Sunshine <sunshine@sunshineco.com>
>>> Cc: Taylor Blau <me@ttaylorr.com>
> 
> The Cc annotations shouldn't be part of the commit message. If you want
> to Cc specific folks you should rather do it e.g. on the command line or
> whatever you use to send out the patches. Otherwise, these will all end
> up in our history.
> 
>>> ---
>>>   daemon.c    | 14 +++++++++-----
>>>   imap-send.c | 13 ++++++++-----
>>>   merge-ll.c  |  6 ++----
>>>   3 files changed, 19 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/daemon.c b/daemon.c
>>> index cb946e3c95f..3fdb6e83c40 100644
>>> --- a/daemon.c
>>> +++ b/daemon.c
>>> @@ -1308,17 +1308,21 @@ int cmd_main(int argc, const char **argv)
>>>                          continue;
>>>                  }
>>>                  if (skip_prefix(arg, "--timeout=", &v)) {
>>> -                       timeout = atoi(v);
>>> +                       if (strtoul_ui(v, 10, &timeout) < 0) {
>>> +                               die("'%s': not a valid integer for --timeout", v);
>>> +                       }
>>>                          continue;
>>>                  }
> 
> We don't use braces around single-line statements. It would also help to
> explain whether this is fixing a bug and, if it does, then it would be
> nice to have a testcase that demonstrates the behaviour. The same is
> true for the other sites that you convert.
> 
> [snip]
>> I also want to ask if this is the right way to send another patch as I
>> noticed that it is showing my previous patch which is not related to
>> this. Thank you.
> 
> You shouldn't ever include patches from another patch series. I guess
> tha problem here is that you created all of your work on the same
> branch. I'd recommend to use separate feature branches for every series
> you are working on. In general, these branches should start from the
> current "main" branch.
> 
> Patrick
>