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

phillip.wood123@xxxxxxxxx · Mon, 14 Oct 2024 19:30:40 +0100

On 14/10/2024 19:20, Usman Akinyemi wrote:
On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:
From: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx>
               }
               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(...)
ohh, what I understand in this part of the code is intended to set
max_connections to 0 if the value it is currently set to is invalid,
such as containing letters or being negative. Your suggestion implies
that we should return an error to indicate that letters are not
accepted.

Yes - I don't think we should be accepting any old rubbish when we 
expect a number

Best Wishes

Phillip

         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









