Thread (94 messages) 94 messages, 6 authors, 2024-11-06

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

From: Usman Akinyemi <hidden>
Date: 2024-10-13 09:42:54

On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
[off-list ref] wrote:
quoted hunk ↗ jump to hunk
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
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help