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

>
> Hi Usman
>
> On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:
> > 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.
>
> 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@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>
>
> 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
>