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@xxxxxxxxx> wrote:
From: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx>
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@xxxxxxxxxxxxxx/
Signed-off-by: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx>
Cc: gitster@xxxxxxxxx
Cc: Patrick Steinhardt <ps@xxxxxx>
Cc: phillip.wood123@xxxxxxxxx
Cc: Christian Couder <christian.couder@xxxxxxxxx>
Cc: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
Cc: Taylor Blau <me@xxxxxxxxxxxx>
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
