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

["Usman Akinyemi via GitGitGadget" <gitgitgadget@gmail.com>]

Changes from Version 1:

 * In my initial commit, I mistakenly included changes from a different
   patch and commit. This issue has now been resolved.
 * I have split the original commit into three separate patches for better
   clarity and organization.
 * I added corresponding tests for each of the changes to ensure proper
   functionality.
 * In the first version, I used the following logic: if (strtoul_ui(v, 10,
   &timeout) == 0) Based on feedback from my mentor, I improved it to:
   (strtoul_ui(v, 10, &timeout)) and similar cases.

Usman Akinyemi (3):
  daemon: replace atoi() with strtoul_ui() and strtol_i()
  merge: replace atoi() with strtol_i() for marker size validation
  imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT
    parsing

 daemon.c              | 11 +++++++----
 imap-send.c           | 13 ++++++++-----
 merge-ll.c            |  6 ++++--
 t/t5570-git-daemon.sh | 27 ++++++++++++++++++++++++++-
 t/t6406-merge-attr.sh |  7 +++++++
 5 files changed, 52 insertions(+), 12 deletions(-)


base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1810%2FUnique-Usman%2Fr_atoi-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1810/Unique-Usman/r_atoi-v2
Pull-Request: https://github.com/git/git/pull/1810

Range-diff vs v1:

 1:  bfff7937cd2 < -:  ----------- t3404: avoid losing exit status with focus on `git show` and `git cat-file`
 2:  e2cae7f3a51 < -:  ----------- t3404: replace test with test_line_count()
 -:  ----------- > 1:  a333d8a4013 daemon: replace atoi() with strtoul_ui() and strtol_i()
 -:  ----------- > 2:  5d58c150efb merge: replace atoi() with strtol_i() for marker size validation
 3:  c93bc2d81ff ! 3:  c09c7b3df0d parse: replace atoi() with strtoul_ui() and strtol_i()
     @@ Metadata
      Author: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
       ## Commit message ##
     -    parse: replace atoi() with strtoul_ui() and strtol_i()
     +    imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing
      
     -    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/
     +    Replaced unsafe uses of atoi() with strtol_i() to improve error handling
     +    when parsing UIDVALIDITY, UIDNEXT, and APPENDUID in IMAP commands.
     +    Invalid values, such as those with letters,
     +    now trigger error messages and prevent malformed status responses.
      
          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 ##
     -@@ daemon.c: 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")) {
     -
       ## imap-send.c ##
      @@ imap-send.c: 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) {
     ++		if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity) {
       			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) {
     ++		if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) || !imap->uidnext) {
       			fprintf(stderr, "IMAP error: malformed NEXTUID status\n");
       			return RESP_BAD;
       		}
     @@ imap-send.c: static int parse_response_code(struct imap_store *ctx, struct imap_
       	} 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)) {
     ++		if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity) ||
     ++			!(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) || !cb->ctx)) {
       			fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
       			return RESP_BAD;
       		}
     @@ imap-send.c: static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *
       				return DRV_OK;
       		} else {
      -			tag = atoi(arg);
     -+			if (strtol_i(arg, 10, &tag) != 0) {
     ++			if (strtol_i(arg, 10, &tag)) {
      +				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;
     -
     - ## merge-ll.c ##
     -@@ merge-ll.c: 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);
     -@@ merge-ll.c: 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