[PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i()
From: Usman Akinyemi via GitGitGadget <hidden>
Date: 2024-10-12 23:09:40
Subsystem:
the rest · Maintainer:
Linus Torvalds
From: Usman Akinyemi <redacted> 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 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 <redacted> Cc: gitster@pobox.com Cc: Patrick Steinhardt <redacted> Cc: phillip.wood123@gmail.com Cc: Christian Couder <redacted> Cc: Eric Sunshine <redacted> Cc: Taylor Blau <redacted> --- 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; } 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); + } 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) { + 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) { 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) { 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)) { 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) { + fprintf(stderr, "IMAP error: malformed tag %s\n", arg); + return RESP_BAD; + } 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) 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) marker_size = DEFAULT_CONFLICT_MARKER_SIZE; } return marker_size;
--
gitgitgadget