On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood [off-list ref] wrote:

Hi Usman

On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:

quoted

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.

This paragraph is good as it explains why you are making this change

quoted

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.

quoted

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

quoted

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>

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.

quoted

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

quoted

+                             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".

quoted

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

quoted

+                     }
                      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 */

quoted

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

Hello, regarding this. I used strtol_i here as ctx->uidvalidity
was declared to be int so, the strtoul_ui complained as it was
expecting an unsigned int.
My suggestion will be to leave it as strol_i and use this comparison
(strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity), what do you think ?

[1] https://www.rfc-editor.org/rfc/rfc3501#section-2.3.1.1

quoted

                      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

quoted

                      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

quoted

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

quoted

+                             fprintf(stderr, "IMAP error: malformed tag %s\n", arg);
+                             return RESP_BAD;

This matches the error below so I assume it's good.

quoted

+                     }
                      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.

quoted

                      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