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

>
> 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
>
> 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>
> ---
>  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