Hi Usman
On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget 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.
This paragraph is good as it explains why you are making this change
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
This information is not really needed in the commit message as it is
shown in the diff.
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.
Great
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>
We do not tend to use Cc: footers on this list. Also note that as there
is a blank line between the Signed-off-by: line and this paragraph the
Signed-off-by: will be ignored by git-interpret-trailers.
---
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) {
For functions that return 0 or -1 to indicate success or error
respectively we use "if (func(args))" to check for errors.
+ die("'%s': not a valid integer for --timeout", v);
"-1" is a valid integer but it is not a valid timeout, maybe we could
say something like "invalid timeout '%s', expecting a non-negative integer".
+ }
continue;
}
if (skip_prefix(arg, "--init-timeout=", &v)) {
- init_timeout = atoi(v);
+ if (strtoul_ui(v, 10, &init_timeout) < 0) {
+ die("'%s': not a valid integer for --init-timeout", v);
The comments for --timeout apply here as well
+ }
continue;
}
if (skip_prefix(arg, "--max-connections=", &v)) {
- max_connections = atoi(v);
- if (max_connections < 0)
- max_connections = 0; /* unlimited */
+ if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) {
This is a faithful translation but if the aim of this series is to
detect errors then I think we want to do something like
if (strtol_i(v, 10, &max_connections))
die(...)
if (max_connections < 0)
max_connections = 0; /* unlimited */
+ max_connections = 0; /* unlimited */
+ }
continue;
}
if (!strcmp(arg, "--strict-paths")) {
diff --git a/imap-send.c b/imap-send.c
index ec68a066877..33b74dfded7 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -668,12 +668,12 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
return RESP_BAD;
}
if (!strcmp("UIDVALIDITY", arg)) {
- if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
+ if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) != 0) {
The original is checking for a zero return from atoi() which indicates
an error or that the parsed value was zero. To do that with strtol_i()
we need to do
|| (strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity)
The IMAP RFC[1] specifies that UIDVALIDITY should be a non-zero,
non-negative 32bit integer but I'm not sure we want to start change it's
type and using strtoul_ui here.
[1] https://www.rfc-editor.org/rfc/rfc3501#section-2.3.1.1
fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
return RESP_BAD;
}
} else if (!strcmp("UIDNEXT", arg)) {
- if (!(arg = next_arg(&s)) || !(imap->uidnext = atoi(arg))) {
+ if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) != 0) {
The comments above apply here
fprintf(stderr, "IMAP error: malformed NEXTUID status\n");
return RESP_BAD;
}
@@ -686,8 +686,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
for (; isspace((unsigned char)*p); p++);
fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
} else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
- if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
- !(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
+ if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) != 0) ||
+ !(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) != 0)) {
And here
fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
return RESP_BAD;
}
@@ -773,7 +773,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
if (!tcmd)
return DRV_OK;
} else {
- tag = atoi(arg);
+ if (strtol_i(arg, 10, &tag) != 0) {
To check for an error just use (strtol_i(arg, 10, &tag))
+ fprintf(stderr, "IMAP error: malformed tag %s\n", arg);
+ return RESP_BAD;
This matches the error below so I assume it's good.
+ }
for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
if (cmdp->tag == tag)
goto gottag;
diff --git a/merge-ll.c b/merge-ll.c
index 8e63071922b..2bfee0f2c6b 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -427,8 +427,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
git_check_attr(istate, path, check);
ll_driver_name = check->items[0].value;
if (check->items[1].value) {
- marker_size = atoi(check->items[1].value);
- if (marker_size <= 0)
+ if (strtol_i(check->items[1].value, 10, &marker_size) != 0 || marker_size <= 0)
Here I think we want to return an error if we cannot parse the marker
size and then set the default if the marker size is <= 0 like we do for
the max_connections code in daemon.c above.
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
driver = find_ll_merge_driver(ll_driver_name);
@@ -454,8 +453,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
check = attr_check_initl("conflict-marker-size", NULL);
git_check_attr(istate, path, check);
if (check->items[0].value) {
- marker_size = atoi(check->items[0].value);
- if (marker_size <= 0)
+ if (strtol_i(check->items[0].value, 10, &marker_size) != 0 || marker_size <= 0)
And the same here
Thanks for working on this, it will be a useful improvement to our
integer parsing. I think you've got the basic idea, it just needs a bit
of polish
Phillip
