[v3,0/3] parse: replace atoi() with strtoul_ui() and strtol_i()

Message

Usman Akinyemi via GitGitGadget Oct. 22, 2024, 5:23 a.m. UTC

Changes from Version 2:

 * Use ${SQ} for single quote.
 * Change the commit message from Updated to Update, Replaced to Replace.
 * Format the commit message well.
 * Used warning for when marker size contains letters instead of die to
   avoid breaking somebody elses command as the test involve adding
   conflict_marker_size into .gitiattribute which is commited into the
   repository.

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            | 11 +++++++++--
 t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++
 t/t6406-merge-attr.sh |  6 ++++++
 5 files changed, 56 insertions(+), 11 deletions(-)


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

Range-diff vs v2:

 1:  a333d8a4013 ! 1:  e292b82d6a1 daemon: replace atoi() with strtoul_ui() and strtol_i()
     @@ Metadata
       ## Commit message ##
          daemon: replace atoi() with strtoul_ui() and strtol_i()
      
     -    Replaced atoi() with strtoul_ui() for --timeout and --init-timeout
     +    Replace atoi() with strtoul_ui() for --timeout and --init-timeout
          (non-negative integers) and with strtol_i() for --max-connections
          (signed integers). This improves error handling and input validation
          by detecting invalid values and providing clear error messages.
     -    Updated tests to ensure these arguments are properly validated.
     +    Update tests to ensure these arguments are properly validated.
      
          Signed-off-by: Usman Akinyemi <[e-mail]>
      
     @@ daemon.c: int cmd_main(int argc, const char **argv)
       		if (!strcmp(arg, "--strict-paths")) {
      
       ## t/t5570-git-daemon.sh ##
     -@@
     - #!/bin/sh
     - 
     --test_description='test fetching over git protocol'
     -+test_description='test fetching over git protocol and daemon rejects invalid options'
     - GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
     - export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
     - 
      @@ t/t5570-git-daemon.sh: TEST_PASSES_SANITIZE_LEAK=true
       . ./test-lib.sh
       
     @@ t/t5570-git-daemon.sh: TEST_PASSES_SANITIZE_LEAK=true
      +	for arg in "3a" "-3"
      +	do
      +		test_must_fail git daemon --init-timeout="$arg" 2>actual_error &&
     -+		test_write_lines "fatal: invalid init-timeout '\''$arg'\'', expecting a non-negative integer" >expected &&
     ++		test_write_lines "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
      +		test_cmp actual_error expected || return 1
      +	done
      +'
     @@ t/t5570-git-daemon.sh: TEST_PASSES_SANITIZE_LEAK=true
      +	for arg in "3a" "-3"
      +	do
      +		test_must_fail git daemon --timeout="$arg" 2>actual_error &&
     -+		test_write_lines "fatal: invalid timeout '\''$arg'\'', expecting a non-negative integer" >expected &&
     ++		test_write_lines "fatal: invalid timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
      +		test_cmp actual_error expected || return 1
      +	done
      +'
      +
      +test_expect_success 'daemon rejects invalid --max-connections values' '
     ++	arg='3a' &&
      +	test_must_fail git daemon --max-connections=3a 2>actual_error &&
     -+	test_write_lines "fatal: invalid max-connections '\''3a'\'', expecting an integer" >expected &&
     ++	test_write_lines "fatal: invalid max-connections ${SQ}$arg${SQ}, expecting an integer" >expected &&
      +	test_cmp actual_error expected
      +'
      +
 2:  5d58c150efb ! 2:  2ad3b0faa05 merge: replace atoi() with strtol_i() for marker size validation
     @@ Metadata
       ## Commit message ##
          merge: replace atoi() with strtol_i() for marker size validation
      
     -    Replaced atoi() with strtol_i() for parsing conflict-marker-size to
     +    Replace atoi() with strtol_i() for parsing conflict-marker-size to
          improve error handling. Invalid values, such as those containing letters
          now trigger a clear error message.
     -    Updated the test to verify invalid input handling.
     +    Update the test to verify invalid input handling.
      
          Signed-off-by: Usman Akinyemi <[e-mail]>
      
       ## merge-ll.c ##
     +@@
     + #include "merge-ll.h"
     + #include "quote.h"
     + #include "strbuf.h"
     ++#include "gettext.h"
     + 
     + struct ll_merge_driver;
     + 
      @@ 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 (strtol_i(check->items[1].value, 10, &marker_size))
     -+			die("invalid marker-size '%s', expecting an integer", check->items[1].value);
     ++		if (strtol_i(check->items[1].value, 10, &marker_size)) {
     ++			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
     ++			warning(_("invalid marker-size '%s', expecting an integer"), check->items[1].value);
     ++		}
       		if (marker_size <= 0)
       			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
       	}
     @@ merge-ll.c: int ll_merge_marker_size(struct index_state *istate, const char *pat
       	git_check_attr(istate, path, check);
       	if (check->items[0].value) {
      -		marker_size = atoi(check->items[0].value);
     -+		if (strtol_i(check->items[0].value, 10, &marker_size))
     -+			die("invalid marker-size '%s', expecting an integer", check->items[0].value);
     ++		if (strtol_i(check->items[0].value, 10, &marker_size)) {
     ++			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
     ++			warning(_("invalid marker-size '%s', expecting an integer"), check->items[0].value);
     ++		}
       		if (marker_size <= 0)
       			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
       	}
     @@ t/t6406-merge-attr.sh: test_expect_success 'retry the merge with longer context'
       
      +test_expect_success 'invalid conflict-marker-size 3a' '
      +    echo "text conflict-marker-size=3a" >>.gitattributes &&
     -+    test_must_fail git checkout -m text 2>actual_error &&
     -+    test_write_lines "fatal: invalid marker-size '\''3a'\'', expecting an integer" >expected &&
     -+    test_cmp actual_error expected
     ++    git checkout -m text 2>error &&
     ++    test_grep "warning: invalid marker-size ${SQ}3a${SQ}, expecting an integer" error
      +'
      +
       test_expect_success 'custom merge backend' '
 3:  c09c7b3df0d ! 3:  d0aa756d2d0 imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing
     @@ Metadata
       ## Commit message ##
          imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing
      
     -    Replaced unsafe uses of atoi() with strtol_i() to improve error handling
     +    Replace 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.
     +    Invalid values, such as those with letters, now trigger error messages and
     +    prevent malformed status responses.
     +    I did not add any test for this commit as we do not have any test
     +    for git-imap-send(1) at this point.
      
          Signed-off-by: Usman Akinyemi <[e-mail]>