[PATCH 0/3] R atoi

[PATCH 0/3] R atoi

[Usman Akinyemi via GitGitGadget]

parse: replace atoi() with strtoul_ui() and strtol_i()

Usman Akinyemi (3):
  t3404: avoid losing exit status with focus on `git show` and `git
    cat-file`
  t3404: replace test with test_line_count()
  parse: replace atoi() with strtoul_ui() and strtol_i()

 daemon.c                      | 14 ++++---
 imap-send.c                   | 13 +++---
 merge-ll.c                    |  6 +--
 t/t3404-rebase-interactive.sh | 75 +++++++++++++++++++++++------------
 4 files changed, 69 insertions(+), 39 deletions(-)


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

[PATCH 1/3] t3404: avoid losing exit status with focus on `git show` and `git cat-file`

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

The exit code of the preceding command in a pipe is disregarded. So
if that preceding command is a Git command that fails, the test would
not fail. Instead, by saving the output of that Git command to a file,
and removing the pipe, we make sure the test will fail if that Git
command fails. This particular patch focuses on all `git show` and
some instances of `git cat-file`.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index f171af3061d..96a65783c47 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -319,7 +319,8 @@ test_expect_success 'retain authorship' '
 	GIT_AUTHOR_NAME="Twerp Snog" git commit -m "different author" &&
 	git tag twerp &&
 	git rebase -i --onto primary HEAD^ &&
-	git show HEAD | grep "^Author: Twerp Snog"
+	git show HEAD >actual &&
+	grep "^Author: Twerp Snog" actual
 '
 
 test_expect_success 'retain authorship w/ conflicts' '
@@ -360,7 +361,8 @@ test_expect_success 'squash' '
 '
 
 test_expect_success 'retain authorship when squashing' '
-	git show HEAD | grep "^Author: Twerp Snog"
+	git show HEAD >actual &&
+	grep "^Author: Twerp Snog" actual
 '
 
 test_expect_success '--continue tries to commit' '
@@ -374,7 +376,8 @@ test_expect_success '--continue tries to commit' '
 		FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue
 	) &&
 	test_cmp_rev HEAD^ new-branch1 &&
-	git show HEAD | grep chouette
+	git show HEAD >actual &&
+	grep chouette actual
 '
 
 test_expect_success 'verbose flag is heeded, even after --continue' '
@@ -397,7 +400,9 @@ test_expect_success 'multi-squash only fires up editor once' '
 			git rebase -i $base
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
-	test 1 = $(git show | grep ONCE | wc -l)
+	git show >output &&
+	count=$(grep ONCE output | wc -l) &&
+	test 1 = $count
 '
 
 test_expect_success 'multi-fixup does not fire up editor' '
@@ -410,7 +415,9 @@ test_expect_success 'multi-fixup does not fire up editor' '
 			git rebase -i $base
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
-	test 0 = $(git show | grep NEVER | wc -l) &&
+	git show >output &&
+	count=$(grep NEVER output | wc -l) &&
+	test 0 = $count &&
 	git checkout @{-1} &&
 	git branch -D multi-fixup
 '
@@ -428,7 +435,9 @@ test_expect_success 'commit message used after conflict' '
 			git rebase --continue
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
-	test 1 = $(git show | grep ONCE | wc -l) &&
+	git show >output &&
+	count=$(grep ONCE output | wc -l) &&
+	test 1 = $count &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
 '
@@ -446,7 +455,9 @@ test_expect_success 'commit message retained after conflict' '
 			git rebase --continue
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
-	test 2 = $(git show | grep TWICE | wc -l) &&
+	git show >output &&
+	count=$(grep TWICE output | wc -l) &&
+	test 2 = $count &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
 '
@@ -470,10 +481,10 @@ test_expect_success 'squash and fixup generate correct log messages' '
 	) &&
 	git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fixup &&
 	test_cmp expect-squash-fixup actual-squash-fixup &&
-	git cat-file commit HEAD@{2} |
-		grep "^# This is a combination of 3 commits\."  &&
-	git cat-file commit HEAD@{3} |
-		grep "^# This is a combination of 2 commits\."  &&
+	git cat-file commit HEAD@{2} >actual &&
+	grep "^# This is a combination of 3 commits\." actual &&
+	git cat-file commit HEAD@{3} >actual &&
+	grep "^# This is a combination of 2 commits\." actual  &&
 	git checkout @{-1} &&
 	git branch -D squash-fixup
 '
@@ -489,7 +500,9 @@ test_expect_success 'squash ignores comments' '
 			git rebase -i $base
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
-	test 1 = $(git show | grep ONCE | wc -l) &&
+	git show >output &&
+	count=$(grep ONCE output | wc -l) &&
+	test 1 = $count &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
 '
@@ -505,7 +518,9 @@ test_expect_success 'squash ignores blank lines' '
 			git rebase -i $base
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
-	test 1 = $(git show | grep ONCE | wc -l) &&
+	git show >output &&
+	count=$(grep ONCE output | wc -l) &&
+	test 1 = $count &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
 '
@@ -572,7 +587,8 @@ test_expect_success '--continue tries to commit, even for "edit"' '
 		FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue
 	) &&
 	test edited = $(git show HEAD:file7) &&
-	git show HEAD | grep chouette &&
+	git show HEAD >actual &&
+	grep chouette actual &&
 	test $parent = $(git rev-parse HEAD^)
 '
 
@@ -757,19 +773,23 @@ test_expect_success 'reword' '
 		set_fake_editor &&
 		FAKE_LINES="1 2 3 reword 4" FAKE_COMMIT_MESSAGE="E changed" \
 			git rebase -i A &&
-		git show HEAD | grep "E changed" &&
+		git show HEAD >actual &&
+		grep "E changed" actual &&
 		test $(git rev-parse primary) != $(git rev-parse HEAD) &&
 		test_cmp_rev primary^ HEAD^ &&
 		FAKE_LINES="1 2 reword 3 4" FAKE_COMMIT_MESSAGE="D changed" \
 			git rebase -i A &&
-		git show HEAD^ | grep "D changed" &&
+		git show HEAD^ >actual &&
+		grep "D changed" actual &&
 		FAKE_LINES="reword 1 2 3 4" FAKE_COMMIT_MESSAGE="B changed" \
 			git rebase -i A &&
-		git show HEAD~3 | grep "B changed" &&
+		git show HEAD~3 >actual &&
+		grep "B changed" actual &&
 		FAKE_LINES="1 r 2 pick 3 p 4" FAKE_COMMIT_MESSAGE="C changed" \
 			git rebase -i A
 	) &&
-	git show HEAD~2 | grep "C changed"
+	git show HEAD~2 >actual &&
+	grep "C changed" actual
 '
 
 test_expect_success 'no uncommitted changes when rewording and the todo list is reloaded' '
@@ -1003,8 +1023,10 @@ test_expect_success 'rebase -i --root retain root commit author and message' '
 		set_fake_editor &&
 		FAKE_LINES="2" git rebase -i --root
 	) &&
-	git cat-file commit HEAD | grep -q "^author Twerp Snog" &&
-	git cat-file commit HEAD | grep -q "^different author$"
+	git cat-file commit HEAD >output &&
+	grep -q "^author Twerp Snog" output &&
+	git cat-file commit HEAD >actual &&
+	grep -q "^different author$" actual
 '
 
 test_expect_success 'rebase -i --root temporary sentinel commit' '
@@ -1013,7 +1035,8 @@ test_expect_success 'rebase -i --root temporary sentinel commit' '
 		set_fake_editor &&
 		test_must_fail env FAKE_LINES="2" git rebase -i --root
 	) &&
-	git cat-file commit HEAD | grep "^tree $EMPTY_TREE" &&
+	git cat-file commit HEAD >actual &&
+	grep "^tree $EMPTY_TREE" actual &&
 	git rebase --abort
 '
 
@@ -1036,7 +1059,8 @@ test_expect_success 'rebase -i --root reword original root commit' '
 		FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
 			git rebase -i --root
 	) &&
-	git show HEAD^ | grep "A changed" &&
+	git show HEAD^ >actual &&
+	grep "A changed" actual &&
 	test -z "$(git show -s --format=%p HEAD^)"
 '
 
@@ -1048,7 +1072,8 @@ test_expect_success 'rebase -i --root reword new root commit' '
 		FAKE_LINES="reword 3 1" FAKE_COMMIT_MESSAGE="C changed" \
 		git rebase -i --root
 	) &&
-	git show HEAD^ | grep "C changed" &&
+	git show HEAD^ >actual &&
+	grep "C changed" actual &&
 	test -z "$(git show -s --format=%p HEAD^)"
 '
 
-- 
gitgitgadget

[PATCH 2/3] t3404: replace test with test_line_count()

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Refactor t3404 to replace instances of `test` with `test_line_count()`
for checking line counts. This improves readability and aligns with Git's
current test practices.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a65783c47..2ab660ef30f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -281,8 +281,9 @@ test_expect_success 'stop on conflicting pick' '
 	test_cmp expect2 file1 &&
 	test "$(git diff --name-status |
 		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
-	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
-	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
+	grep -v "^#" <.git/rebase-merge/done >actual &&
+	test_line_count = 4 actual &&
+	test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
 '
 
 test_expect_success 'show conflicted patch' '
@@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count
+	grep ONCE output >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'multi-fixup does not fire up editor' '
@@ -416,8 +417,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep NEVER output | wc -l) &&
-	test 0 = $count &&
+	! grep NEVER output &&
 	git checkout @{-1} &&
 	git branch -D multi-fixup
 '
@@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
 '
@@ -456,8 +456,8 @@ test_expect_success 'commit message retained after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep TWICE output | wc -l) &&
-	test 2 = $count &&
+	grep TWICE output >actual &&
+	test_line_count = 2 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
 '
@@ -501,8 +501,8 @@ test_expect_success 'squash ignores comments' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
 '
@@ -519,8 +519,8 @@ test_expect_success 'squash ignores blank lines' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
 '
-- 
gitgitgadget

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

[Usman Akinyemi via GitGitGadget]

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.

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/

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    | 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) {
+				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")) {
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) {
 			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) {
 			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)) {
 			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) {
+				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;
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)
 			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)
 			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	}
 	return marker_size;
-- 
gitgitgadget

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

[Usman Akinyemi]

On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
<gitgitgadget@gmail.com> 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.
>
> 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/
>
> 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    | 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) {
> +                               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")) {
> 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) {
>                         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) {
>                         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)) {
>                         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) {
> +                               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;
> 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)
>                         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)
>                         marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>         }
>         return marker_size;
> --
> gitgitgadget

I also want to ask if this is the right way to send another patch as I
noticed that it is showing my previous patch which is not related to
this. Thank you.

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

[Phillip Wood]

Hi Usman

On 13/10/2024 10:42, Usman Akinyemi wrote:
> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> 
> I also want to ask if this is the right way to send another patch as I
> noticed that it is showing my previous patch which is not related to
> this. Thank you.

When you start working on a new patch series you should create a new 
branch from origin/master with

     git switch -c my-new-branch origin/master

that way your new work will be based on Junio's master branch rather 
than your other patch series. You can use

     git branch --set-upstream-to origin/master
     git rebase HEAD^

to drop the first two patches and set the correct upstream for your branch.

Best Wishes

Phillip

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

[Phillip Wood]

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

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

[Kristoffer Haugsbakk]

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

I thought that gitgitgadget checked for missing sign-off.  I’ve seen
that message before at least.

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

[Patrick Steinhardt]

On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> <gitgitgadget@gmail.com> 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.
> >
> > 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

To me it's always an indicator that something should be split up across
multiple commits once you have a bulleted list of changes in your commit
message.

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

The Cc annotations shouldn't be part of the commit message. If you want
to Cc specific folks you should rather do it e.g. on the command line or
whatever you use to send out the patches. Otherwise, these will all end
up in our history.

> > ---
> >  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) {
> > +                               die("'%s': not a valid integer for --timeout", v);
> > +                       }
> >                         continue;
> >                 }

We don't use braces around single-line statements. It would also help to
explain whether this is fixing a bug and, if it does, then it would be
nice to have a testcase that demonstrates the behaviour. The same is
true for the other sites that you convert.

[snip]
> I also want to ask if this is the right way to send another patch as I
> noticed that it is showing my previous patch which is not related to
> this. Thank you.

You shouldn't ever include patches from another patch series. I guess
tha problem here is that you created all of your work on the same
branch. I'd recommend to use separate feature branches for every series
you are working on. In general, these branches should start from the
current "main" branch.

Patrick

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

[Phillip Wood]

Hi Kristoffer

On 14/10/2024 11:06, Kristoffer Haugsbakk wrote:
>>> 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.
> 
> I thought that gitgitgadget checked for missing sign-off.  I’ve seen
> that message before at least.

I'm not sure what the DCO check does as I can't figure out what code its 
running, but it looks like the commit lint just uses a regex on the 
whole commit message[1]. I think the check could be tightened up to 
ensure there is a Signed-off-by line that matches the commit author as I 
seem to recall we've sometimes seen SOB lines with another identity instead.

Best Wishes

Phillip

[1] 
https://github.com/gitgitgadget/gitgitgadget/blob/7726b025bfaa18b72c889ae01f053d77d34f199d/lib/commit-lint.ts#L142

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

[Phillip Wood]

On 14/10/2024 11:53, Patrick Steinhardt wrote:
> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
>> <gitgitgadget@gmail.com> 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.
>>>
>>> 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
> 
> To me it's always an indicator that something should be split up across
> multiple commits once you have a bulleted list of changes in your commit
> message.

Agreed, but I think in this case there is a common theme (converting 
atoi() to a safer alternative) and the problem is with the commit 
message listing which files have changed rather than unrelated code 
changes being grouped together. This patch could be split up and if 
there were many more atoi() conversions it would need to be split to 
prevent it being too long but I don't think its essential to do so.

Best Wishes

Phillip

>>> 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/
>>>
>>> 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>
> 
> The Cc annotations shouldn't be part of the commit message. If you want
> to Cc specific folks you should rather do it e.g. on the command line or
> whatever you use to send out the patches. Otherwise, these will all end
> up in our history.
> 
>>> ---
>>>   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) {
>>> +                               die("'%s': not a valid integer for --timeout", v);
>>> +                       }
>>>                          continue;
>>>                  }
> 
> We don't use braces around single-line statements. It would also help to
> explain whether this is fixing a bug and, if it does, then it would be
> nice to have a testcase that demonstrates the behaviour. The same is
> true for the other sites that you convert.
> 
> [snip]
>> I also want to ask if this is the right way to send another patch as I
>> noticed that it is showing my previous patch which is not related to
>> this. Thank you.
> 
> You shouldn't ever include patches from another patch series. I guess
> tha problem here is that you created all of your work on the same
> branch. I'd recommend to use separate feature branches for every series
> you are working on. In general, these branches should start from the
> current "main" branch.
> 
> Patrick
> 

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

[Patrick Steinhardt]

On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:
> On 14/10/2024 11:53, Patrick Steinhardt wrote:
> > On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> > > On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> > > <gitgitgadget@gmail.com> 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.
> > > > 
> > > > 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
> > 
> > To me it's always an indicator that something should be split up across
> > multiple commits once you have a bulleted list of changes in your commit
> > message.
> 
> Agreed, but I think in this case there is a common theme (converting atoi()
> to a safer alternative) and the problem is with the commit message listing
> which files have changed rather than unrelated code changes being grouped
> together. This patch could be split up and if there were many more atoi()
> conversions it would need to be split to prevent it being too long but I
> don't think its essential to do so.

In theory I agree. In practice I think we should have better
explanations why the respective conversions are fine and whether this is
fixing a bug or not. And if it is fixing bugs I'd also like to see tests
added to the tree.

And by the time we got there it makes sense to split up commits.

Patrick

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

[Phillip Wood]

On 14/10/2024 15:00, Patrick Steinhardt wrote:
> On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:
>> On 14/10/2024 11:53, Patrick Steinhardt wrote:
>>> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
>>>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
>>>> <gitgitgadget@gmail.com> 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.
>>>>>
>>>>> 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
>>>
>>> To me it's always an indicator that something should be split up across
>>> multiple commits once you have a bulleted list of changes in your commit
>>> message.
>>
>> Agreed, but I think in this case there is a common theme (converting atoi()
>> to a safer alternative) and the problem is with the commit message listing
>> which files have changed rather than unrelated code changes being grouped
>> together. This patch could be split up and if there were many more atoi()
>> conversions it would need to be split to prevent it being too long but I
>> don't think its essential to do so.
> 
> In theory I agree. In practice I think we should have better
> explanations why the respective conversions are fine and whether this is
> fixing a bug or not. And if it is fixing bugs I'd also like to see tests
> added to the tree.

I'm not sure if I would describe any of the changes as fixing bugs. The 
option and config parsing code becomes stricter so I guess you could say 
it was a bug to accept any old rubbish and treat it as zero before. The 
imap code that's changed all rejected zero anyway apart from the tag 
parsing so maybe accepting the changes to the tag parsing are fixing a bug.

> And by the time we got there it makes sense to split up commits.

Yes if we start adding tests then it is worth splitting them up, I'm not 
sure we have anyway of testing the imap changes but it would be worth 
testing the other changes though.

Phillip

> Patrick
> 

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

[Usman Akinyemi]

On Mon, Oct 14, 2024 at 9:00 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Usman
>
> On 13/10/2024 10:42, Usman Akinyemi wrote:
> > On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> >
> > I also want to ask if this is the right way to send another patch as I
> > noticed that it is showing my previous patch which is not related to
> > this. Thank you.
>
> When you start working on a new patch series you should create a new
> branch from origin/master with
>
>      git switch -c my-new-branch origin/master
>
> that way your new work will be based on Junio's master branch rather
> than your other patch series. You can use
>
>      git branch --set-upstream-to origin/master
>      git rebase HEAD^
>
> to drop the first two patches and set the correct upstream for your branch.
>
> Best Wishes
>
> Phillip
>
Thanks Philip, I actually created another branch but I was really
confused if to base the new branch on master or the branch which has
the previous commits. Thanks for clarifying this.

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

[Usman Akinyemi]

On Mon, Oct 14, 2024 at 10:53 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> > On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> > <gitgitgadget@gmail.com> 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.
> > >
> > > 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
>
> To me it's always an indicator that something should be split up across
> multiple commits once you have a bulleted list of changes in your commit
> message.
>
> > > 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/
> > >
> > > 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>
>
> The Cc annotations shouldn't be part of the commit message. If you want
> to Cc specific folks you should rather do it e.g. on the command line or
> whatever you use to send out the patches. Otherwise, these will all end
> up in our history.
Thanks for this, I thought the gitgitgadget automatically use the Cc
from the commit message.
>
> > > ---
> > >  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) {
> > > +                               die("'%s': not a valid integer for --timeout", v);
> > > +                       }
> > >                         continue;
> > >                 }
>
> We don't use braces around single-line statements. It would also help to
> explain whether this is fixing a bug and, if it does, then it would be
> nice to have a testcase that demonstrates the behaviour. The same is
> true for the other sites that you convert.
>
I was going to add testcase, I sent this patch to ensure I am going in
the right direction.
> [snip]
> > I also want to ask if this is the right way to send another patch as I
> > noticed that it is showing my previous patch which is not related to
> > this. Thank you.
>
> You shouldn't ever include patches from another patch series. I guess
> tha problem here is that you created all of your work on the same
> branch. I'd recommend to use separate feature branches for every series
> you are working on. In general, these branches should start from the
> current "main" branch.
>
> Patrick
Thanks Patrick. I actually created a new branch for this branch, my
mistake was not basing it on the master branch. I was a little bit
confused. But, now I understand better. Thanks.

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

[Usman Akinyemi]

On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 14/10/2024 15:00, Patrick Steinhardt wrote:
> > On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:
> >> On 14/10/2024 11:53, Patrick Steinhardt wrote:
> >>> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> >>>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> >>>> <gitgitgadget@gmail.com> 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.
> >>>>>
> >>>>> 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
> >>>
> >>> To me it's always an indicator that something should be split up across
> >>> multiple commits once you have a bulleted list of changes in your commit
> >>> message.
> >>
> >> Agreed, but I think in this case there is a common theme (converting atoi()
> >> to a safer alternative) and the problem is with the commit message listing
> >> which files have changed rather than unrelated code changes being grouped
> >> together. This patch could be split up and if there were many more atoi()
> >> conversions it would need to be split to prevent it being too long but I
> >> don't think its essential to do so.
> >
> > In theory I agree. In practice I think we should have better
> > explanations why the respective conversions are fine and whether this is
> > fixing a bug or not. And if it is fixing bugs I'd also like to see tests
> > added to the tree.
>
> I'm not sure if I would describe any of the changes as fixing bugs. The
> option and config parsing code becomes stricter so I guess you could say
> it was a bug to accept any old rubbish and treat it as zero before. The
> imap code that's changed all rejected zero anyway apart from the tag
> parsing so maybe accepting the changes to the tag parsing are fixing a bug.
>
> > And by the time we got there it makes sense to split up commits.
>
> Yes if we start adding tests then it is worth splitting them up, I'm not
> sure we have anyway of testing the imap changes but it would be worth
> testing the other changes though.
>
> Phillip
>
> > Patrick
> >
>
I got this from a leftoverbit which the main issue was reported as
bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/

For the test, I should have the test as another patch right ?
Thanks.

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

[Usman Akinyemi]

On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
>
> On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> > On 14/10/2024 15:00, Patrick Steinhardt wrote:
> > > On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:
> > >> On 14/10/2024 11:53, Patrick Steinhardt wrote:
> > >>> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> > >>>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> > >>>> <gitgitgadget@gmail.com> 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.
> > >>>>>
> > >>>>> 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
> > >>>
> > >>> To me it's always an indicator that something should be split up across
> > >>> multiple commits once you have a bulleted list of changes in your commit
> > >>> message.
> > >>
> > >> Agreed, but I think in this case there is a common theme (converting atoi()
> > >> to a safer alternative) and the problem is with the commit message listing
> > >> which files have changed rather than unrelated code changes being grouped
> > >> together. This patch could be split up and if there were many more atoi()
> > >> conversions it would need to be split to prevent it being too long but I
> > >> don't think its essential to do so.
> > >
> > > In theory I agree. In practice I think we should have better
> > > explanations why the respective conversions are fine and whether this is
> > > fixing a bug or not. And if it is fixing bugs I'd also like to see tests
> > > added to the tree.
> >
> > I'm not sure if I would describe any of the changes as fixing bugs. The
> > option and config parsing code becomes stricter so I guess you could say
> > it was a bug to accept any old rubbish and treat it as zero before. The
> > imap code that's changed all rejected zero anyway apart from the tag
> > parsing so maybe accepting the changes to the tag parsing are fixing a bug.
> >
> > > And by the time we got there it makes sense to split up commits.
> >
> > Yes if we start adding tests then it is worth splitting them up, I'm not
> > sure we have anyway of testing the imap changes but it would be worth
> > testing the other changes though.
> >
> > Phillip
> >
> > > Patrick
> > >
> >
> I got this from a leftoverbit which the main issue was reported as
> bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/
>
> For the test, I should have the test as another patch right ?
> Thanks.
Also, do I need to add the reference which mentions the leftoverbit in
the commit message?

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

[Usman Akinyemi]

On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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(...)
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.
>         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
>

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

[phillip.wood123]

On 14/10/2024 19:20, Usman Akinyemi wrote:
> On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:
>>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>>>                }
>>>                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
>>

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

[phillip.wood123]

On 14/10/2024 17:26, Usman Akinyemi wrote:
> On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi
>> On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> I got this from a leftoverbit which the main issue was reported as
>> bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/
>> For the test, I should have the test as another patch right ?

In general you should add tests in the same commit as the code changes 
that they test. In this instance I think you want to split this patch 
into three, one patch for git-daemon, one for imap-send and one for the 
merge marker config changes. Each patch should have a commit message 
explaining the changes and whether they change the behavior of the code 
(for example rejecting non-numbers) and add some tests. Note that I 
don't think it is possible to test the imap-send changes but the other 
two should be easy enough. The tests should be added to one of the 
existing test files that are testing the code being changed.

>> Thanks.
> Also, do I need to add the reference which mentions the leftoverbit in
> the commit message?

I'm not sure that's necessary so long as you explain the reason for the 
changes in the commit message.


Best Wishes

Phillip

Re: [PATCH 1/3] t3404: avoid losing exit status with focus on `git show` and `git cat-file`

[Taylor Blau]

On Sat, Oct 12, 2024 at 11:09:32PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>
> The exit code of the preceding command in a pipe is disregarded. So
> if that preceding command is a Git command that fails, the test would
> not fail. Instead, by saving the output of that Git command to a file,
> and removing the pipe, we make sure the test will fail if that Git
> command fails. This particular patch focuses on all `git show` and
> some instances of `git cat-file`.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
>  1 file changed, 48 insertions(+), 23 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index f171af3061d..96a65783c47 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -319,7 +319,8 @@ test_expect_success 'retain authorship' '
>  	GIT_AUTHOR_NAME="Twerp Snog" git commit -m "different author" &&
>  	git tag twerp &&
>  	git rebase -i --onto primary HEAD^ &&
> -	git show HEAD | grep "^Author: Twerp Snog"
> +	git show HEAD >actual &&
> +	grep "^Author: Twerp Snog" actual
>  '

Good.

> @@ -397,7 +400,9 @@ test_expect_success 'multi-squash only fires up editor once' '
>  			git rebase -i $base
>  	) &&
>  	test $base = $(git rev-parse HEAD^) &&
> -	test 1 = $(git show | grep ONCE | wc -l)
> +	git show >output &&
> +	count=$(grep ONCE output | wc -l) &&
> +	test 1 = $count
>  '

I think moving 'git show' out of the pipeline is a good step here, but I
don't think we need to store $count in a separate variable. It would be
fine to write:

    git show >output &&
    test 1 = $(grep ONCE output | wc -l)

or even to replace the subshell with 'grep -c' instead of piping 'grep'
to 'wc -l'.

>  test_expect_success 'multi-fixup does not fire up editor' '
> @@ -410,7 +415,9 @@ test_expect_success 'multi-fixup does not fire up editor' '
>  			git rebase -i $base
>  	) &&
>  	test $base = $(git rev-parse HEAD^) &&
> -	test 0 = $(git show | grep NEVER | wc -l) &&
> +	git show >output &&
> +	count=$(grep NEVER output | wc -l) &&
> +	test 0 = $count &&
>  	git checkout @{-1} &&
>  	git branch -D multi-fixup
>  '

Same notes from above here and the next two tests (elided from my
response) below.

> @@ -470,10 +481,10 @@ test_expect_success 'squash and fixup generate correct log messages' '
>  	) &&
>  	git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fixup &&
>  	test_cmp expect-squash-fixup actual-squash-fixup &&
> -	git cat-file commit HEAD@{2} |
> -		grep "^# This is a combination of 3 commits\."  &&
> -	git cat-file commit HEAD@{3} |
> -		grep "^# This is a combination of 2 commits\."  &&
> +	git cat-file commit HEAD@{2} >actual &&
> +	grep "^# This is a combination of 3 commits\." actual &&

Is there a more descriptive name for the output here than just 'actual'?

Thanks,
Taylor

Re: [PATCH 2/3] t3404: replace test with test_line_count()

[Taylor Blau]

On Sat, Oct 12, 2024 at 11:09:33PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>
> Refactor t3404 to replace instances of `test` with `test_line_count()`
> for checking line counts. This improves readability and aligns with Git's
> current test practices.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 96a65783c47..2ab660ef30f 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -281,8 +281,9 @@ test_expect_success 'stop on conflicting pick' '
>  	test_cmp expect2 file1 &&
>  	test "$(git diff --name-status |
>  		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
> -	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
> -	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
> +	grep -v "^#" <.git/rebase-merge/done >actual &&
> +	test_line_count = 4 actual &&
> +	test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)

You use 'test_line_count' in one instance here, but 'test 0 =' below.
You could use 'test_must_be_empty' to stick with our test_-helper
functions.

But I like that you used 'grep -c' here, so it may be better to match
the two like so:

    test 4 $(grep -c -v "^#" <.git/rebase-merge/done) &&
    test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)

> @@ -416,8 +417,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
>  	) &&
>  	test $base = $(git rev-parse HEAD^) &&
>  	git show >output &&
> -	count=$(grep NEVER output | wc -l) &&
> -	test 0 = $count &&
> +	! grep NEVER output &&
>  	git checkout @{-1} &&
>  	git branch -D multi-fixup
>  '

Hmm. Wasn't this modified by the previous step as well? Is there a
reason that these can't be combined to avoid a new intermediate state
that will be thrown away in the next step?

> @@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' '
>  	) &&
>  	test $base = $(git rev-parse HEAD^) &&
>  	git show >output &&
> -	count=$(grep ONCE output | wc -l) &&
> -	test 1 = $count &&
> +	grep ONCE output >actual &&
> +	test_line_count = 1 actual &&
>  	git checkout @{-1} &&
>  	git branch -D conflict-fixup

I am not sure what the benefit of using test_line_count here is over
bare 'test'. Can you explain why you chose to use it here?

In the body of your patch above, you appear to suggest that using
test_line_count is more in the style of Git's current test practices. I
think that's true for cases like writing:

    test_line_count = 1 actual

as opposed to:

    test 1 = $(wc -l <actual)

Since the former doesn't have the gotcha that you must remember redirect
the input of 'wc -l' to avoid having the filename appear in the output,
and the former also ensures that the file exists, has better error
messages, etc.

But in the case where we're running 'grep -c' directly, it seems cleaner
to use bare test, since we're not writing the matches to a file on disk.

Thanks,
Taylor

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

[Usman Akinyemi]

On Mon, Oct 14, 2024 at 6:36 PM <phillip.wood123@gmail.com> wrote:
>
> On 14/10/2024 17:26, Usman Akinyemi wrote:
> > On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi
> >> On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >> I got this from a leftoverbit which the main issue was reported as
> >> bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/
> >> For the test, I should have the test as another patch right ?
>
> In general you should add tests in the same commit as the code changes
> that they test. In this instance I think you want to split this patch
> into three, one patch for git-daemon, one for imap-send and one for the
> merge marker config changes. Each patch should have a commit message
> explaining the changes and whether they change the behavior of the code
> (for example rejecting non-numbers) and add some tests. Note that I
> don't think it is possible to test the imap-send changes but the other
> two should be easy enough. The tests should be added to one of the
> existing test files that are testing the code being changed.
>
Hello, thanks for this, I was working on this and I need help. For the
merge-ll.c,
I noticed that the check->items[0].value were already checked to
ensure they do not contain letters in them.
        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 expecting an integer");
                if (marker_size <= 0)
                        marker_size = DEFAULT_CONFLICT_MARKER_SIZE

error: option `marker-size' expects a numerical value
not ok 38 - merge without conflict wrong marker-size
#
# cp new1.txt test.txt &&
# test_must_fail git merge-file -p --marker-size=1a test.txt orig.txt
new2.txt 2>error &&
# cat error &&
#     grep "invalid" error
#
I grepped the error message and I noticed that the message is gotten
from parse-options.c and it ensures that the arg is negative. How to
proceed in such a case ?

Also, for the daemon.c I am finding
it hard to get the exact test file to add the new test.

Thank you.
Usman Akinyemi


> >> Thanks.
> > Also, do I need to add the reference which mentions the leftoverbit in
> > the commit message?
>
> I'm not sure that's necessary so long as you explain the reason for the
> changes in the commit message.
>
>
> Best Wishes
>
> Phillip
>
>

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

[Taylor Blau]

On Tue, Oct 15, 2024 at 03:17:05PM +0000, Usman Akinyemi wrote:
> Also, for the daemon.c I am finding
> it hard to get the exact test file to add the new test.

t5570-git-daemon.sh is the test file I usually think of for adding the
most direct tests exercising git-daemon.

If you're ever unsure, I find it useful to grep through the filenames of
scripts in t, like so:

    $ ls t/t????-*.sh | grep daemon
    t/t5570-git-daemon.sh

Thanks,
Taylor

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

[phillip.wood123]

Hi Usman

On 15/10/2024 16:17, Usman Akinyemi wrote:
> On Mon, Oct 14, 2024 at 6:36 PM <phillip.wood123@gmail.com> wrote:
>>
>> On 14/10/2024 17:26, Usman Akinyemi wrote:
>>> On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi
>>>> On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>> I got this from a leftoverbit which the main issue was reported as
>>>> bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/
>>>> For the test, I should have the test as another patch right ?
>>
>> In general you should add tests in the same commit as the code changes
>> that they test. In this instance I think you want to split this patch
>> into three, one patch for git-daemon, one for imap-send and one for the
>> merge marker config changes. Each patch should have a commit message
>> explaining the changes and whether they change the behavior of the code
>> (for example rejecting non-numbers) and add some tests. Note that I
>> don't think it is possible to test the imap-send changes but the other
>> two should be easy enough. The tests should be added to one of the
>> existing test files that are testing the code being changed.
>>
> Hello, thanks for this, I was working on this and I need help. For the
> merge-ll.c,
> I noticed that the check->items[0].value were already checked to
> ensure they do not contain letters in them.
>          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 expecting an integer");
>                  if (marker_size <= 0)
>                          marker_size = DEFAULT_CONFLICT_MARKER_SIZE
> 
> error: option `marker-size' expects a numerical value
> not ok 38 - merge without conflict wrong marker-size
> #
> # cp new1.txt test.txt &&
> # test_must_fail git merge-file -p --marker-size=1a test.txt orig.txt
> new2.txt 2>error &&
> # cat error &&
> #     grep "invalid" error

It would be better to check for the error message with test_cmp or at 
least grep for a longer phrase so we're sure the error message is the 
one we think we should be getting.

> #
> I grepped the error message and I noticed that the message is gotten
> from parse-options.c and it ensures that the arg is negative. How to
> proceed in such a case ?

The code you're changing parses the conflict-marker-size attribute so 
you need to set up a .gitattributes file with an invalid marker size and 
then run "git merge" or "git cherry-pick"

Best Wishes

Phillip

> Also, for the daemon.c I am finding
> it hard to get the exact test file to add the new test.
> 
> Thank you.
> Usman Akinyemi
> 
> 
>>>> Thanks.
>>> Also, do I need to add the reference which mentions the leftoverbit in
>>> the commit message?
>>
>> I'm not sure that's necessary so long as you explain the reason for the
>> changes in the commit message.
>>
>>
>> Best Wishes
>>
>> Phillip
>>
>>

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

[Phillip Wood]

On 15/10/2024 19:28, phillip.wood123@gmail.com wrote:
> On 15/10/2024 16:17, Usman Akinyemi wrote:
>> 
>> I grepped the error message and I noticed that the message is gotten
>> from parse-options.c and it ensures that the arg is negative. How to
>> proceed in such a case ?
> 
> The code you're changing parses the conflict-marker-size attribute so 
> you need to set up a .gitattributes file with an invalid marker size and 
> then run "git merge" or "git cherry-pick"

t/t6406-merge-attr.sh would be a good place to add this test

Best Wishes

Phillip

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

[Usman Akinyemi]

On Tue, Oct 15, 2024 at 4:19 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Oct 15, 2024 at 03:17:05PM +0000, Usman Akinyemi wrote:
> > Also, for the daemon.c I am finding
> > it hard to get the exact test file to add the new test.
>
> t5570-git-daemon.sh is the test file I usually think of for adding the
> most direct tests exercising git-daemon.
>
> If you're ever unsure, I find it useful to grep through the filenames of
> scripts in t, like so:
>
>     $ ls t/t????-*.sh | grep daemon
>     t/t5570-git-daemon.sh
>
> Thanks,
> Taylor
Thanks for this, I really appreciate it.

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

[Usman Akinyemi]

On Wed, Oct 16, 2024 at 9:20 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 15/10/2024 19:28, phillip.wood123@gmail.com wrote:
> > On 15/10/2024 16:17, Usman Akinyemi wrote:
> >>
> >> I grepped the error message and I noticed that the message is gotten
> >> from parse-options.c and it ensures that the arg is negative. How to
> >> proceed in such a case ?
> >
> > The code you're changing parses the conflict-marker-size attribute so
> > you need to set up a .gitattributes file with an invalid marker size and
> > then run "git merge" or "git cherry-pick"
Thanks.
>
> t/t6406-merge-attr.sh would be a good place to add this test
Thank you Philip.
>
> Best Wishes
>
> Phillip
>

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

[Usman Akinyemi]

On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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.
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
>
> >                       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
>

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

[Usman Akinyemi]

On Mon, Oct 14, 2024 at 6:36 PM <phillip.wood123@gmail.com> wrote:
>
> On 14/10/2024 17:26, Usman Akinyemi wrote:
> > On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi
> >> On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >> I got this from a leftoverbit which the main issue was reported as
> >> bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/
> >> For the test, I should have the test as another patch right ?
>
> In general you should add tests in the same commit as the code changes
> that they test. In this instance I think you want to split this patch
> into three, one patch for git-daemon, one for imap-send and one for the
> merge marker config changes. Each patch should have a commit message
> explaining the changes and whether they change the behavior of the code
> (for example rejecting non-numbers) and add some tests. Note that I
> don't think it is possible to test the imap-send changes but the other
> two should be easy enough. The tests should be added to one of the
> existing test files that are testing the code being changed.
Hello,
I am currently facing some issues while trying to write the test for
daemon.c, I need some help on it.
The start_git_daemon function inside lib-git-daemon.sh is made to
allow --init-timeout, --max-connections and
timeout as well as other arguments. The start_git_daemon function in
lib-git-daemon.sh is used at t5570-git-daemon.sh.
Basically this is my changes
                if (skip_prefix(arg, "--timeout=", &v)) {
-                       timeout = atoi(v);
+                       if (strtoul_ui(v, 10, &timeout))
+                               die("invalid timeout '%s', expecting a
non-negative integer", v);
                        continue;
                }
                if (skip_prefix(arg, "--init-timeout=", &v)) {
-                       init_timeout = atoi(v);
+                       if (strtoul_ui(v, 10, &init_timeout))
+                               die("invalid init-timeout '%s',
expecting a non-negative integer", v);
                        continue;
                }
                if (skip_prefix(arg, "--max-connections=", &v)) {
-                       max_connections = atoi(v);
+                       if (strtol_i(v, 10, &max_connections))
+                               die("invalid '--max-connections' '%s',
expecting an integer", v);
                        if (max_connections < 0)
-                               max_connections = 0;            /* unlimited */
+                               max_connections = 0;  /* unlimited */
                        continue;
                }
What happened is that the start_git_daemon will already fail and will
prevent the
t5570-git-daemon.sh from starting if there is any wrong starting
condition such as the new
changes I added. I am finding it hard to come up with an approach to
test the new change.


Thank you.
>
> >> Thanks.
> > Also, do I need to add the reference which mentions the leftoverbit in
> > the commit message?
>
> I'm not sure that's necessary so long as you explain the reason for the
> changes in the commit message.
>
>
> Best Wishes
>
> Phillip
>
>

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

[Patrick Steinhardt]

On Thu, Oct 17, 2024 at 11:56:33AM +0000, Usman Akinyemi wrote:
> On Mon, Oct 14, 2024 at 6:36 PM <phillip.wood123@gmail.com> wrote:
> >
> > On 14/10/2024 17:26, Usman Akinyemi wrote:
> > > On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi
> > >> On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > >> I got this from a leftoverbit which the main issue was reported as
> > >> bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/
> > >> For the test, I should have the test as another patch right ?
> >
> > In general you should add tests in the same commit as the code changes
> > that they test. In this instance I think you want to split this patch
> > into three, one patch for git-daemon, one for imap-send and one for the
> > merge marker config changes. Each patch should have a commit message
> > explaining the changes and whether they change the behavior of the code
> > (for example rejecting non-numbers) and add some tests. Note that I
> > don't think it is possible to test the imap-send changes but the other
> > two should be easy enough. The tests should be added to one of the
> > existing test files that are testing the code being changed.
> Hello,
> I am currently facing some issues while trying to write the test for
> daemon.c, I need some help on it.
> The start_git_daemon function inside lib-git-daemon.sh is made to
> allow --init-timeout, --max-connections and
> timeout as well as other arguments. The start_git_daemon function in
> lib-git-daemon.sh is used at t5570-git-daemon.sh.
> Basically this is my changes
>                 if (skip_prefix(arg, "--timeout=", &v)) {
> -                       timeout = atoi(v);
> +                       if (strtoul_ui(v, 10, &timeout))
> +                               die("invalid timeout '%s', expecting a
> non-negative integer", v);
>                         continue;
>                 }
>                 if (skip_prefix(arg, "--init-timeout=", &v)) {
> -                       init_timeout = atoi(v);
> +                       if (strtoul_ui(v, 10, &init_timeout))
> +                               die("invalid init-timeout '%s',
> expecting a non-negative integer", v);
>                         continue;
>                 }
>                 if (skip_prefix(arg, "--max-connections=", &v)) {
> -                       max_connections = atoi(v);
> +                       if (strtol_i(v, 10, &max_connections))
> +                               die("invalid '--max-connections' '%s',
> expecting an integer", v);
>                         if (max_connections < 0)
> -                               max_connections = 0;            /* unlimited */
> +                               max_connections = 0;  /* unlimited */
>                         continue;
>                 }
> What happened is that the start_git_daemon will already fail and will
> prevent the
> t5570-git-daemon.sh from starting if there is any wrong starting
> condition such as the new
> changes I added. I am finding it hard to come up with an approach to
> test the new change.

I'd just not use `start_git_daemon ()` in the first place. Instead, I'd
invoke git-daemon(1) directly with invalid options and then observe that
it fails to start up with the expected error message.

Patrick

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

[Usman Akinyemi]

On Thu, Oct 17, 2024 at 12:02 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Oct 17, 2024 at 11:56:33AM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 14, 2024 at 6:36 PM <phillip.wood123@gmail.com> wrote:
> > >
> > > On 14/10/2024 17:26, Usman Akinyemi wrote:
> > > > On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi
> > > >> On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > > >> I got this from a leftoverbit which the main issue was reported as
> > > >> bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/
> > > >> For the test, I should have the test as another patch right ?
> > >
> > > In general you should add tests in the same commit as the code changes
> > > that they test. In this instance I think you want to split this patch
> > > into three, one patch for git-daemon, one for imap-send and one for the
> > > merge marker config changes. Each patch should have a commit message
> > > explaining the changes and whether they change the behavior of the code
> > > (for example rejecting non-numbers) and add some tests. Note that I
> > > don't think it is possible to test the imap-send changes but the other
> > > two should be easy enough. The tests should be added to one of the
> > > existing test files that are testing the code being changed.
> > Hello,
> > I am currently facing some issues while trying to write the test for
> > daemon.c, I need some help on it.
> > The start_git_daemon function inside lib-git-daemon.sh is made to
> > allow --init-timeout, --max-connections and
> > timeout as well as other arguments. The start_git_daemon function in
> > lib-git-daemon.sh is used at t5570-git-daemon.sh.
> > Basically this is my changes
> >                 if (skip_prefix(arg, "--timeout=", &v)) {
> > -                       timeout = atoi(v);
> > +                       if (strtoul_ui(v, 10, &timeout))
> > +                               die("invalid timeout '%s', expecting a
> > non-negative integer", v);
> >                         continue;
> >                 }
> >                 if (skip_prefix(arg, "--init-timeout=", &v)) {
> > -                       init_timeout = atoi(v);
> > +                       if (strtoul_ui(v, 10, &init_timeout))
> > +                               die("invalid init-timeout '%s',
> > expecting a non-negative integer", v);
> >                         continue;
> >                 }
> >                 if (skip_prefix(arg, "--max-connections=", &v)) {
> > -                       max_connections = atoi(v);
> > +                       if (strtol_i(v, 10, &max_connections))
> > +                               die("invalid '--max-connections' '%s',
> > expecting an integer", v);
> >                         if (max_connections < 0)
> > -                               max_connections = 0;            /* unlimited */
> > +                               max_connections = 0;  /* unlimited */
> >                         continue;
> >                 }
> > What happened is that the start_git_daemon will already fail and will
> > prevent the
> > t5570-git-daemon.sh from starting if there is any wrong starting
> > condition such as the new
> > changes I added. I am finding it hard to come up with an approach to
> > test the new change.
>
> I'd just not use `start_git_daemon ()` in the first place. Instead, I'd
> invoke git-daemon(1) directly with invalid options and then observe that
> it fails to start up with the expected error message.
>
> Patrick
Hello Patrick, thanks for the reply. that works, I really appreciate it.

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

[Usman Akinyemi via GitGitGadget]

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

[PATCH v2 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 daemon.c              | 11 +++++++----
 t/t5570-git-daemon.sh | 27 ++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/daemon.c b/daemon.c
index cb946e3c95f..09a31d2344d 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1308,17 +1308,20 @@ int cmd_main(int argc, const char **argv)
 			continue;
 		}
 		if (skip_prefix(arg, "--timeout=", &v)) {
-			timeout = atoi(v);
+			if (strtoul_ui(v, 10, &timeout))
+				die("invalid timeout '%s', expecting a non-negative integer", v);
 			continue;
 		}
 		if (skip_prefix(arg, "--init-timeout=", &v)) {
-			init_timeout = atoi(v);
+			if (strtoul_ui(v, 10, &init_timeout))
+				die("invalid init-timeout '%s', expecting a non-negative integer", v);
 			continue;
 		}
 		if (skip_prefix(arg, "--max-connections=", &v)) {
-			max_connections = atoi(v);
+			if (strtol_i(v, 10, &max_connections))
+				die("invalid max-connections '%s', expecting an integer", v);
 			if (max_connections < 0)
-				max_connections = 0;	        /* unlimited */
+				max_connections = 0;  /* unlimited */
 			continue;
 		}
 		if (!strcmp(arg, "--strict-paths")) {
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index c5f08b67996..c73c2196981 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -1,6 +1,6 @@
 #!/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
 
@@ -8,6 +8,31 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
+
+test_expect_success 'daemon rejects invalid --init-timeout values' '
+	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_cmp actual_error expected || return 1
+	done
+'
+
+test_expect_success 'daemon rejects invalid --timeout values' '
+	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_cmp actual_error expected || return 1
+	done
+'
+
+test_expect_success 'daemon rejects invalid --max-connections values' '
+	test_must_fail git daemon --max-connections=3a 2>actual_error &&
+	test_write_lines "fatal: invalid max-connections '\''3a'\'', expecting an integer" >expected &&
+	test_cmp actual_error expected
+'
+
 start_git_daemon
 
 check_verbose_connect () {
-- 
gitgitgadget

[PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 merge-ll.c            | 6 ++++--
 t/t6406-merge-attr.sh | 7 +++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 8e63071922b..52870226816 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -427,7 +427,8 @@ 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 (marker_size <= 0)
 			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	}
@@ -454,7 +455,8 @@ 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 (strtol_i(check->items[0].value, 10, &marker_size))
+			die("invalid marker-size '%s', expecting an integer", check->items[0].value);
 		if (marker_size <= 0)
 			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	}
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 9bf95249347..1299b30aeb1 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -118,6 +118,13 @@ test_expect_success 'retry the merge with longer context' '
 	grep "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<" actual
 '
 
+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
+'
+
 test_expect_success 'custom merge backend' '
 
 	echo "* merge=union" >.gitattributes &&
-- 
gitgitgadget

[PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@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>
---
 imap-send.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index ec68a066877..8214df128e5 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) || !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) || !imap->uidnext) {
 			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) || !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;
 		}
@@ -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)) {
+				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;
-- 
gitgitgadget

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

[Taylor Blau]

On Fri, Oct 18, 2024 at 01:52:57PM +0000, Usman Akinyemi via GitGitGadget wrote:
> Changes from Version 1:
>
>  * In my initial commit, I mistakenly included changes from a different
>    patch and commit. This issue has now been resolved.

Should we treat this as a new series, then? Or is this a true reroll of
the previous round and should be kept together?

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

Thanks,
Taylor

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

[Usman Akinyemi]

On Fri, Oct 18, 2024 at 9:22 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Fri, Oct 18, 2024 at 01:52:57PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > Changes from Version 1:
> >
> >  * In my initial commit, I mistakenly included changes from a different
> >    patch and commit. This issue has now been resolved.
>
> Should we treat this as a new series, then? Or is this a true reroll of
> the previous round and should be kept together?
Hello Taylor,
Yeah, this should be treated as a new series different from the two below.
 - t3404: replace test with test_line_count()
 - t3404: avoid losing exit status with focus on `git show` and `git cat-file`
Thank you.
>
> >  * 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
>
> Thanks,
> Taylor

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

[Taylor Blau]

On Fri, Oct 18, 2024 at 09:29:44PM +0000, Usman Akinyemi wrote:
> On Fri, Oct 18, 2024 at 9:22 PM Taylor Blau <me@ttaylorr.com> wrote:
> >
> > On Fri, Oct 18, 2024 at 01:52:57PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > Changes from Version 1:
> > >
> > >  * In my initial commit, I mistakenly included changes from a different
> > >    patch and commit. This issue has now been resolved.
> >
> > Should we treat this as a new series, then? Or is this a true reroll of
> > the previous round and should be kept together?
> Hello Taylor,
> Yeah, this should be treated as a new series different from the two below.
>  - t3404: replace test with test_line_count()
>  - t3404: avoid losing exit status with focus on `git show` and `git cat-file`

Gotcha. So in the original ua/t3404-cleanup series from my tree, I
should drop the third and final patch:

  parse: replace atoi() with strtoul_ui() and strtol_i()

?

Thanks,
Taylor

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

[Usman Akinyemi]

On Fri, Oct 18, 2024 at 9:36 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Fri, Oct 18, 2024 at 09:29:44PM +0000, Usman Akinyemi wrote:
> > On Fri, Oct 18, 2024 at 9:22 PM Taylor Blau <me@ttaylorr.com> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:52:57PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > Changes from Version 1:
> > > >
> > > >  * In my initial commit, I mistakenly included changes from a different
> > > >    patch and commit. This issue has now been resolved.
> > >
> > > Should we treat this as a new series, then? Or is this a true reroll of
> > > the previous round and should be kept together?
> > Hello Taylor,
> > Yeah, this should be treated as a new series different from the two below.
> >  - t3404: replace test with test_line_count()
> >  - t3404: avoid losing exit status with focus on `git show` and `git cat-file`
>
> Gotcha. So in the original ua/t3404-cleanup series from my tree, I
> should drop the third and final patch:
>
>   parse: replace atoi() with strtoul_ui() and strtol_i()
>
> ?
Hello Taylor,
Yeah, exactly. Thank you.
Usman Akinyemi.
>
> Thanks,
> Taylor

Re: [PATCH v2 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 01:52:58PM +0000, Usman Akinyemi via GitGitGadget wrote:
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index c5f08b67996..c73c2196981 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -1,6 +1,6 @@
>  #!/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
>  

Hum. I think the test description can stay as-is, as we don't typically
mention all the exact details of what we test in a test suite. But I
also don't mind this too much.

> @@ -8,6 +8,31 @@ TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  . "$TEST_DIRECTORY"/lib-git-daemon.sh
> +
> +test_expect_success 'daemon rejects invalid --init-timeout values' '
> +	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 &&

You can use ${SQ} instead of '\'', also for the other two tests.

Patrick

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> 
> Replaced 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.

When starting a new paragraph we typically have an empty line between
the paragraphs. We also tend to write commit messages as if instructing
the code to change. So instead of "Replaced atoi() with..." you'd say
"Replace atoi() with", and instead of "Updated the test...", you'd say
"Update the test ...".

The same applies to your other commits, as well.

> 
> diff --git a/merge-ll.c b/merge-ll.c
> index 8e63071922b..52870226816 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -427,7 +427,8 @@ 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 (marker_size <= 0)
>  			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>  	}
> @@ -454,7 +455,8 @@ 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 (strtol_i(check->items[0].value, 10, &marker_size))
> +			die("invalid marker-size '%s', expecting an integer", check->items[0].value);
>  		if (marker_size <= 0)
>  			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>  	}

These are a bit curious. As your test demonstrates, we retrieve the
values from the "gitattributes" file. And given that the file tends to be
checked into the repository, you can now basically break somebody elses
commands by having an invalid value in there.

That makes me think that we likely shouldn't die here. We may print a
warning, but other than that we should likely continue and use the
DEFAULT_CONFLICT_MARKER_SIZE.

Patrick

Re: [PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@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.

The line break after "letters," is a bit funny.

It would also be nice to point out why this commit doesn't add any new
tests. I guess the answer is that we don't have any tests for
git-imap-send(1) at all, which is too bad, but a fair excuse and not a
problem of your patch. So introducing such tests would be too much to
ask.

Patrick

Re: [PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi]

On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <usmanakinyemi202@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.
>
> The line break after "letters," is a bit funny.
I just noticed that I will change it.
>
> It would also be nice to point out why this commit doesn't add any new
> tests. I guess the answer is that we don't have any tests for
> git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> problem of your patch. So introducing such tests would be too much to
> ask.
I can try, but, why was it not introduced before, is there a reason ?
>
> Patrick

Re: [PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Patrick Steinhardt]

On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > From: Usman Akinyemi <usmanakinyemi202@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.
> >
> > The line break after "letters," is a bit funny.
> I just noticed that I will change it.
> >
> > It would also be nice to point out why this commit doesn't add any new
> > tests. I guess the answer is that we don't have any tests for
> > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > problem of your patch. So introducing such tests would be too much to
> > ask.
> I can try, but, why was it not introduced before, is there a reason ?

I think it's mostly that we'd have to have an IMAP server available to
test sending emails properly, so the test setup would be comparatively
involved. Nobody felt like doing that, and thus we don't have any tests
:)

Patrick

Re: [PATCH v2 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Usman Akinyemi]

On Mon, Oct 21, 2024 at 1:36 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Oct 18, 2024 at 01:52:58PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> > index c5f08b67996..c73c2196981 100755
> > --- a/t/t5570-git-daemon.sh
> > +++ b/t/t5570-git-daemon.sh
> > @@ -1,6 +1,6 @@
> >  #!/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
> >
>
> Hum. I think the test description can stay as-is, as we don't typically
> mention all the exact details of what we test in a test suite. But I
> also don't mind this too much.
Ohh, noted. I just thought the test description does not have anything
about merge.
>
> > @@ -8,6 +8,31 @@ TEST_PASSES_SANITIZE_LEAK=true
> >  . ./test-lib.sh
> >
> >  . "$TEST_DIRECTORY"/lib-git-daemon.sh
> > +
> > +test_expect_success 'daemon rejects invalid --init-timeout values' '
> > +     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 &&
>
> You can use ${SQ} instead of '\'', also for the other two tests.
Will make a change now.
>
> Patrick

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi]

On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> >
> > Replaced 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.
>
> When starting a new paragraph we typically have an empty line between
> the paragraphs. We also tend to write commit messages as if instructing
> the code to change. So instead of "Replaced atoi() with..." you'd say
> "Replace atoi() with", and instead of "Updated the test...", you'd say
> "Update the test ...".
>
> The same applies to your other commits, as well.
>
> >
> > diff --git a/merge-ll.c b/merge-ll.c
> > index 8e63071922b..52870226816 100644
> > --- a/merge-ll.c
> > +++ b/merge-ll.c
> > @@ -427,7 +427,8 @@ 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 (marker_size <= 0)
> >                       marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> >       }
> > @@ -454,7 +455,8 @@ 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 (strtol_i(check->items[0].value, 10, &marker_size))
> > +                     die("invalid marker-size '%s', expecting an integer", check->items[0].value);
> >               if (marker_size <= 0)
> >                       marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> >       }
>
> These are a bit curious. As your test demonstrates, we retrieve the
> values from the "gitattributes" file. And given that the file tends to be
> checked into the repository, you can now basically break somebody elses
> commands by having an invalid value in there.
>
> That makes me think that we likely shouldn't die here. We may print a
> warning, but other than that we should likely continue and use the
> DEFAULT_CONFLICT_MARKER_SIZE.
>
Ohh, I understand. Philip suggested this. For the warning, will I just
use printf statement or what function to print the statement ?
Also, how do I test the print warning statement ?

Thank you.
Usman Akinyemi.
> Patrick

Re: [PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi]

On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <usmanakinyemi202@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.
> > >
> > > The line break after "letters," is a bit funny.
> > I just noticed that I will change it.
> > >
> > > It would also be nice to point out why this commit doesn't add any new
> > > tests. I guess the answer is that we don't have any tests for
> > > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > > problem of your patch. So introducing such tests would be too much to
> > > ask.
> > I can try, but, why was it not introduced before, is there a reason ?
>
> I think it's mostly that we'd have to have an IMAP server available to
> test sending emails properly, so the test setup would be comparatively
> involved. Nobody felt like doing that, and thus we don't have any tests
> :)
Ohh, I see. I have not set up an IMAP server before though. I can take
it up but might require some level of guidance.

Usman Akinyemi.
>
> Patrick

Re: [PATCH v2 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Taylor Blau]

On Mon, Oct 21, 2024 at 01:43:53PM +0000, Usman Akinyemi wrote:
> > Hum. I think the test description can stay as-is, as we don't typically
> > mention all the exact details of what we test in a test suite. But I
> > also don't mind this too much.
>
> Ohh, noted. I just thought the test description does not have anything
> about merge.

Let's leave it as-is, unless you can come up with a new description that
is a little shorter. Absent of that, I think the current description is
fine as-is.

> > > @@ -8,6 +8,31 @@ TEST_PASSES_SANITIZE_LEAK=true
> > >  . ./test-lib.sh
> > >
> > >  . "$TEST_DIRECTORY"/lib-git-daemon.sh
> > > +
> > > +test_expect_success 'daemon rejects invalid --init-timeout values' '
> > > +     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 &&
> >
> > You can use ${SQ} instead of '\'', also for the other two tests.
>
> Will make a change now.

Thanks. ${SQ} is much preferred here, and makes the resulting test much
easier to read.

Thanks,
Taylor

Re: [PATCH v2 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Usman Akinyemi]

On Mon, Oct 21, 2024 at 4:24 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Oct 21, 2024 at 01:43:53PM +0000, Usman Akinyemi wrote:
> > > Hum. I think the test description can stay as-is, as we don't typically
> > > mention all the exact details of what we test in a test suite. But I
> > > also don't mind this too much.
> >
> > Ohh, noted. I just thought the test description does not have anything
> > about merge.
>
> Let's leave it as-is, unless you can come up with a new description that
> is a little shorter. Absent of that, I think the current description is
> fine as-is.
Noted andThanks.
>
> > > > @@ -8,6 +8,31 @@ TEST_PASSES_SANITIZE_LEAK=true
> > > >  . ./test-lib.sh
> > > >
> > > >  . "$TEST_DIRECTORY"/lib-git-daemon.sh
> > > > +
> > > > +test_expect_success 'daemon rejects invalid --init-timeout values' '
> > > > +     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 &&
> > >
> > > You can use ${SQ} instead of '\'', also for the other two tests.
> >
> > Will make a change now.
>
> Thanks. ${SQ} is much preferred here, and makes the resulting test much
> easier to read.
Noted and thanks.
>
> Thanks,
> Taylor

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Taylor Blau]

On Mon, Oct 21, 2024 at 02:24:38PM +0000, Usman Akinyemi wrote:
> On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > >
> > > Replaced 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.
> >
> > When starting a new paragraph we typically have an empty line between
> > the paragraphs. We also tend to write commit messages as if instructing
> > the code to change. So instead of "Replaced atoi() with..." you'd say
> > "Replace atoi() with", and instead of "Updated the test...", you'd say
> > "Update the test ...".
> >
> > The same applies to your other commits, as well.

Thanks for noting, Patrick.

> > These are a bit curious. As your test demonstrates, we retrieve the
> > values from the "gitattributes" file. And given that the file tends to be
> > checked into the repository, you can now basically break somebody elses
> > commands by having an invalid value in there.
> >
> > That makes me think that we likely shouldn't die here. We may print a
> > warning, but other than that we should likely continue and use the
> > DEFAULT_CONFLICT_MARKER_SIZE.
> >
>
> Ohh, I understand. Philip suggested this. For the warning, will I just
> use printf statement or what function to print the statement ?
> Also, how do I test the print warning statement ?

You can use warning() instead of die(), which will also print the
message to stderr. You can redirect stderr to a separate file in your
test, and then grep or test_grep that to ensure that you see the warning
message.

These messages should also be marked for translation (with `_()`), so
the result will look something like:

    if (strtol_i(check->items[0].value, 10, &marker_size))
            warning(_("invalid marker-size '%s', expecting an integer"),
                    check->items[0].value);

Thanks,
Taylor

Re: [PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Taylor Blau]

On Mon, Oct 21, 2024 at 02:38:40PM +0000, Usman Akinyemi wrote:
> On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> > > On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> > > >
> > > > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > > From: Usman Akinyemi <usmanakinyemi202@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.
> > > >
> > > > The line break after "letters," is a bit funny.
> > > I just noticed that I will change it.
> > > >
> > > > It would also be nice to point out why this commit doesn't add any new
> > > > tests. I guess the answer is that we don't have any tests for
> > > > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > > > problem of your patch. So introducing such tests would be too much to
> > > > ask.
> > > I can try, but, why was it not introduced before, is there a reason ?
> >
> > I think it's mostly that we'd have to have an IMAP server available to
> > test sending emails properly, so the test setup would be comparatively
> > involved. Nobody felt like doing that, and thus we don't have any tests
> > :)
> Ohh, I see. I have not set up an IMAP server before though. I can take
> it up but might require some level of guidance.

I think what Patrick is saying is that it's probably not worth the
effort to do so for an automated test, especially if the code change is
trivial by comparison.

Thanks,
Taylor

Re: [PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi]

On Mon, Oct 21, 2024 at 4:35 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Oct 21, 2024 at 02:38:40PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> > > > On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> > > > >
> > > > > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > > > From: Usman Akinyemi <usmanakinyemi202@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.
> > > > >
> > > > > The line break after "letters," is a bit funny.
> > > > I just noticed that I will change it.
> > > > >
> > > > > It would also be nice to point out why this commit doesn't add any new
> > > > > tests. I guess the answer is that we don't have any tests for
> > > > > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > > > > problem of your patch. So introducing such tests would be too much to
> > > > > ask.
> > > > I can try, but, why was it not introduced before, is there a reason ?
> > >
> > > I think it's mostly that we'd have to have an IMAP server available to
> > > test sending emails properly, so the test setup would be comparatively
> > > involved. Nobody felt like doing that, and thus we don't have any tests
> > > :)
> > Ohh, I see. I have not set up an IMAP server before though. I can take
> > it up but might require some level of guidance.
>
> I think what Patrick is saying is that it's probably not worth the
> effort to do so for an automated test, especially if the code change is
> trivial by comparison.
>
> Thanks,
> Taylor
Thanks Taylor. Noted.

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi]

On Mon, Oct 21, 2024 at 4:34 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Oct 21, 2024 at 02:24:38PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > > >
> > > > Replaced 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.
> > >
> > > When starting a new paragraph we typically have an empty line between
> > > the paragraphs. We also tend to write commit messages as if instructing
> > > the code to change. So instead of "Replaced atoi() with..." you'd say
> > > "Replace atoi() with", and instead of "Updated the test...", you'd say
> > > "Update the test ...".
> > >
> > > The same applies to your other commits, as well.
>
> Thanks for noting, Patrick.
>
> > > These are a bit curious. As your test demonstrates, we retrieve the
> > > values from the "gitattributes" file. And given that the file tends to be
> > > checked into the repository, you can now basically break somebody elses
> > > commands by having an invalid value in there.
> > >
> > > That makes me think that we likely shouldn't die here. We may print a
> > > warning, but other than that we should likely continue and use the
> > > DEFAULT_CONFLICT_MARKER_SIZE.
> > >
> >
> > Ohh, I understand. Philip suggested this. For the warning, will I just
> > use printf statement or what function to print the statement ?
> > Also, how do I test the print warning statement ?
>
> You can use warning() instead of die(), which will also print the
> message to stderr. You can redirect stderr to a separate file in your
> test, and then grep or test_grep that to ensure that you see the warning
> message.
>
> These messages should also be marked for translation (with `_()`), so
> the result will look something like:
>
>     if (strtol_i(check->items[0].value, 10, &marker_size))
>             warning(_("invalid marker-size '%s', expecting an integer"),
>                     check->items[0].value);
>
> Thanks,
> Taylor
Thank you, I will make the changes.

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi]

On Mon, Oct 21, 2024 at 4:34 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Oct 21, 2024 at 02:24:38PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > > >
> > > > Replaced 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.
> > >
> > > When starting a new paragraph we typically have an empty line between
> > > the paragraphs. We also tend to write commit messages as if instructing
> > > the code to change. So instead of "Replaced atoi() with..." you'd say
> > > "Replace atoi() with", and instead of "Updated the test...", you'd say
> > > "Update the test ...".
> > >
> > > The same applies to your other commits, as well.
>
> Thanks for noting, Patrick.
>
> > > These are a bit curious. As your test demonstrates, we retrieve the
> > > values from the "gitattributes" file. And given that the file tends to be
> > > checked into the repository, you can now basically break somebody elses
> > > commands by having an invalid value in there.
> > >
> > > That makes me think that we likely shouldn't die here. We may print a
> > > warning, but other than that we should likely continue and use the
> > > DEFAULT_CONFLICT_MARKER_SIZE.
> > >
> >
> > Ohh, I understand. Philip suggested this. For the warning, will I just
> > use printf statement or what function to print the statement ?
> > Also, how do I test the print warning statement ?
>
> You can use warning() instead of die(), which will also print the
> message to stderr. You can redirect stderr to a separate file in your
> test, and then grep or test_grep that to ensure that you see the warning
> message.
>
> These messages should also be marked for translation (with `_()`), so
> the result will look something like:
>
>     if (strtol_i(check->items[0].value, 10, &marker_size))
>             warning(_("invalid marker-size '%s', expecting an integer"),
>                     check->items[0].value);
Hi Taylor, when I try to use this warning(_, I was getting some error
In the editor
warning(_("invalid marker-size '%s', expecting an integer"),
check->items[1].value); Incompatible integer to pointer conversion
passing 'int' to parameter of type 'const char *'
while I tried run make

erge-ll.c: In function ‘ll_merge’:
merge-ll.c:432:33: error: implicit declaration of function ‘_’
[-Wimplicit-function-declaration]
  432 |                         warning(_("invalid marker-size '%s',
expecting an integer"), check->items[1].value);
      |                                 ^
merge-ll.c:432:33: error: passing argument 1 of ‘warning’ makes
pointer from integer without a cast [-Wint-conversion]
  432 |                         warning(_("invalid marker-size '%s',
expecting an integer"), check->items[1].value);
      |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                 |
      |                                 int
In file included from merge-ll.c:9:
git-compat-util.h:691:26: note: expected ‘const char *’ but argument
is of type ‘int’
  691 | void warning(const char *err, ...) __attribute__((format
(printf, 1, 2)));
      |              ~~~~~~~~~~~~^~~


>
> Thanks,
> Taylor

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Taylor Blau]

On Mon, Oct 21, 2024 at 06:00:55PM +0000, Usman Akinyemi wrote:
> Hi Taylor, when I try to use this warning(_, I was getting some error
> In the editor

Let's see...

> erge-ll.c: In function ‘ll_merge’:
> merge-ll.c:432:33: error: implicit declaration of function ‘_’
> [-Wimplicit-function-declaration]
>   432 |                         warning(_("invalid marker-size '%s',
> expecting an integer"), check->items[1].value);
>       |                                 ^

Your compiler is correctly indicating that the error is that the
function '_' is undefined, likely because this file does not include
"gettext.h", which is what defines that function.

Thanks,
Taylor

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

[Usman Akinyemi via GitGitGadget]

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 <usmanakinyemi202@gmail.com>
      
     @@ 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 <usmanakinyemi202@gmail.com>
      
       ## 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 <usmanakinyemi202@gmail.com>
      

-- 
gitgitgadget

[PATCH v3 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
Update tests to ensure these arguments are properly validated.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 daemon.c              | 11 +++++++----
 t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index cb946e3c95f..09a31d2344d 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1308,17 +1308,20 @@ int cmd_main(int argc, const char **argv)
 			continue;
 		}
 		if (skip_prefix(arg, "--timeout=", &v)) {
-			timeout = atoi(v);
+			if (strtoul_ui(v, 10, &timeout))
+				die("invalid timeout '%s', expecting a non-negative integer", v);
 			continue;
 		}
 		if (skip_prefix(arg, "--init-timeout=", &v)) {
-			init_timeout = atoi(v);
+			if (strtoul_ui(v, 10, &init_timeout))
+				die("invalid init-timeout '%s', expecting a non-negative integer", v);
 			continue;
 		}
 		if (skip_prefix(arg, "--max-connections=", &v)) {
-			max_connections = atoi(v);
+			if (strtol_i(v, 10, &max_connections))
+				die("invalid max-connections '%s', expecting an integer", v);
 			if (max_connections < 0)
-				max_connections = 0;	        /* unlimited */
+				max_connections = 0;  /* unlimited */
 			continue;
 		}
 		if (!strcmp(arg, "--strict-paths")) {
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index c5f08b67996..722ddb8b7fa 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -8,6 +8,32 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
+
+test_expect_success 'daemon rejects invalid --init-timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --init-timeout="$arg" 2>actual_error &&
+		test_write_lines "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
+		test_cmp actual_error expected || return 1
+	done
+'
+
+test_expect_success 'daemon rejects invalid --timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --timeout="$arg" 2>actual_error &&
+		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 ${SQ}$arg${SQ}, expecting an integer" >expected &&
+	test_cmp actual_error expected
+'
+
 start_git_daemon
 
 check_verbose_connect () {
-- 
gitgitgadget

[PATCH v3 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
Update the test to verify invalid input handling.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 merge-ll.c            | 11 +++++++++--
 t/t6406-merge-attr.sh |  6 ++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 8e63071922b..62fc625552d 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -15,6 +15,7 @@
 #include "merge-ll.h"
 #include "quote.h"
 #include "strbuf.h"
+#include "gettext.h"
 
 struct ll_merge_driver;
 
@@ -427,7 +428,10 @@ 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)) {
+			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;
 	}
@@ -454,7 +458,10 @@ 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 (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;
 	}
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 9bf95249347..c2a9cf03808 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -118,6 +118,12 @@ test_expect_success 'retry the merge with longer context' '
 	grep "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<" actual
 '
 
+test_expect_success 'invalid conflict-marker-size 3a' '
+    echo "text conflict-marker-size=3a" >>.gitattributes &&
+    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' '
 
 	echo "* merge=union" >.gitattributes &&
-- 
gitgitgadget

[PATCH v3 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
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 <usmanakinyemi202@gmail.com>
---
 imap-send.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index ec68a066877..8214df128e5 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) || !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) || !imap->uidnext) {
 			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) || !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;
 		}
@@ -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)) {
+				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;
-- 
gitgitgadget

Re: [PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi]

On Tue, Oct 22, 2024 at 7:21 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <usmanakinyemi202@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.
> > >
> > > The line break after "letters," is a bit funny.
> > I just noticed that I will change it.
> > >
> > > It would also be nice to point out why this commit doesn't add any new
> > > tests. I guess the answer is that we don't have any tests for
> > > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > > problem of your patch. So introducing such tests would be too much to
> > > ask.
> > I can try, but, why was it not introduced before, is there a reason ?
>
> I think it's mostly that we'd have to have an IMAP server available to
> test sending emails properly, so the test setup would be comparatively
> involved. Nobody felt like doing that, and thus we don't have any tests
> :)
>
> Patrick
>
I made all these changes in version 3 of the patch. Thank you.

Re: [PATCH v3 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Taylor Blau]

On Tue, Oct 22, 2024 at 05:23:41AM +0000, Usman Akinyemi via GitGitGadget wrote:
> diff --git a/daemon.c b/daemon.c
> index cb946e3c95f..09a31d2344d 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1308,17 +1308,20 @@ int cmd_main(int argc, const char **argv)
>  			continue;
>  		}
>  		if (skip_prefix(arg, "--timeout=", &v)) {
> -			timeout = atoi(v);
> +			if (strtoul_ui(v, 10, &timeout))
> +				die("invalid timeout '%s', expecting a non-negative integer", v);

The conversion you made to both (a) use warning() and (b) mark the
string for translation in the second patch were good, but I would have
expected to see them here as well.

Perhaps leaving this one as a die() makes sense, because we are taking
direct input from the user, so invoking 'git daemon' with bogus options
should result in us dying. But these strings should be marked as
translate-able regardless.

Thanks,
Taylor

Re: [PATCH v3 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Usman Akinyemi]

On Tue, Oct 22, 2024 at 4:21 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Oct 22, 2024 at 05:23:41AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > diff --git a/daemon.c b/daemon.c
> > index cb946e3c95f..09a31d2344d 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1308,17 +1308,20 @@ int cmd_main(int argc, const char **argv)
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--timeout=", &v)) {
> > -                     timeout = atoi(v);
> > +                     if (strtoul_ui(v, 10, &timeout))
> > +                             die("invalid timeout '%s', expecting a non-negative integer", v);
>
> The conversion you made to both (a) use warning() and (b) mark the
> string for translation in the second patch were good, but I would have
> expected to see them here as well.
>
> Perhaps leaving this one as a die() makes sense, because we are taking
> direct input from the user, so invoking 'git daemon' with bogus options
> should result in us dying. But these strings should be marked as
> translate-able regardless.
As you said, since the git daemon takes direct input from the user,
compared to the other which takes input from .gitattributes leaving as
die is okay here. I have marked it as translate-able in my fourth
patch. Thank you very much for the review.
Usman Akinyemi.
>
> Thanks,
> Taylor

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

[Usman Akinyemi via GitGitGadget]

Changes from Version 3:

 * Mark the error message strings as translate-able.

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              | 12 ++++++++----
 imap-send.c           | 13 ++++++++-----
 merge-ll.c            | 11 +++++++++--
 t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++
 t/t6406-merge-attr.sh |  6 ++++++
 5 files changed, 57 insertions(+), 11 deletions(-)


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

Range-diff vs v3:

 1:  e292b82d6a1 ! 1:  d9c997d7a9c daemon: replace atoi() with strtoul_ui() and strtol_i()
     @@ Commit message
          Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
       ## daemon.c ##
     +@@
     + #include "abspath.h"
     + #include "config.h"
     + #include "environment.h"
     ++#include "gettext.h"
     + #include "path.h"
     + #include "pkt-line.h"
     + #include "protocol.h"
      @@ 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))
     -+				die("invalid timeout '%s', expecting a non-negative integer", v);
     ++				die(_("invalid timeout '%s', expecting a non-negative integer"), v);
       			continue;
       		}
       		if (skip_prefix(arg, "--init-timeout=", &v)) {
      -			init_timeout = atoi(v);
      +			if (strtoul_ui(v, 10, &init_timeout))
     -+				die("invalid init-timeout '%s', expecting a non-negative integer", v);
     ++				die(_("invalid init-timeout '%s', expecting a non-negative integer"), v);
       			continue;
       		}
       		if (skip_prefix(arg, "--max-connections=", &v)) {
      -			max_connections = atoi(v);
      +			if (strtol_i(v, 10, &max_connections))
     -+				die("invalid max-connections '%s', expecting an integer", v);
     ++				die(_("invalid max-connections '%s', expecting an integer"), v);
       			if (max_connections < 0)
      -				max_connections = 0;	        /* unlimited */
      +				max_connections = 0;  /* unlimited */
 2:  2ad3b0faa05 = 2:  da9ea10e4e1 merge: replace atoi() with strtol_i() for marker size validation
 3:  d0aa756d2d0 = 3:  8982dca646d imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

-- 
gitgitgadget

[PATCH v4 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
Update tests to ensure these arguments are properly validated.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 daemon.c              | 12 ++++++++----
 t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index cb946e3c95f..a40e435c637 100644
--- a/daemon.c
+++ b/daemon.c
@@ -4,6 +4,7 @@
 #include "abspath.h"
 #include "config.h"
 #include "environment.h"
+#include "gettext.h"
 #include "path.h"
 #include "pkt-line.h"
 #include "protocol.h"
@@ -1308,17 +1309,20 @@ int cmd_main(int argc, const char **argv)
 			continue;
 		}
 		if (skip_prefix(arg, "--timeout=", &v)) {
-			timeout = atoi(v);
+			if (strtoul_ui(v, 10, &timeout))
+				die(_("invalid timeout '%s', expecting a non-negative integer"), v);
 			continue;
 		}
 		if (skip_prefix(arg, "--init-timeout=", &v)) {
-			init_timeout = atoi(v);
+			if (strtoul_ui(v, 10, &init_timeout))
+				die(_("invalid init-timeout '%s', expecting a non-negative integer"), v);
 			continue;
 		}
 		if (skip_prefix(arg, "--max-connections=", &v)) {
-			max_connections = atoi(v);
+			if (strtol_i(v, 10, &max_connections))
+				die(_("invalid max-connections '%s', expecting an integer"), v);
 			if (max_connections < 0)
-				max_connections = 0;	        /* unlimited */
+				max_connections = 0;  /* unlimited */
 			continue;
 		}
 		if (!strcmp(arg, "--strict-paths")) {
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index c5f08b67996..722ddb8b7fa 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -8,6 +8,32 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
+
+test_expect_success 'daemon rejects invalid --init-timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --init-timeout="$arg" 2>actual_error &&
+		test_write_lines "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
+		test_cmp actual_error expected || return 1
+	done
+'
+
+test_expect_success 'daemon rejects invalid --timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --timeout="$arg" 2>actual_error &&
+		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 ${SQ}$arg${SQ}, expecting an integer" >expected &&
+	test_cmp actual_error expected
+'
+
 start_git_daemon
 
 check_verbose_connect () {
-- 
gitgitgadget

[PATCH v4 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
Update the test to verify invalid input handling.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 merge-ll.c            | 11 +++++++++--
 t/t6406-merge-attr.sh |  6 ++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 8e63071922b..62fc625552d 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -15,6 +15,7 @@
 #include "merge-ll.h"
 #include "quote.h"
 #include "strbuf.h"
+#include "gettext.h"
 
 struct ll_merge_driver;
 
@@ -427,7 +428,10 @@ 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)) {
+			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;
 	}
@@ -454,7 +458,10 @@ 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 (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;
 	}
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 9bf95249347..c2a9cf03808 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -118,6 +118,12 @@ test_expect_success 'retry the merge with longer context' '
 	grep "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<" actual
 '
 
+test_expect_success 'invalid conflict-marker-size 3a' '
+    echo "text conflict-marker-size=3a" >>.gitattributes &&
+    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' '
 
 	echo "* merge=union" >.gitattributes &&
-- 
gitgitgadget

[PATCH v4 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
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 <usmanakinyemi202@gmail.com>
---
 imap-send.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index ec68a066877..8214df128e5 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) || !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) || !imap->uidnext) {
 			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) || !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;
 		}
@@ -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)) {
+				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;
-- 
gitgitgadget

Re: [PATCH v4 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Patrick Steinhardt]

On Tue, Oct 22, 2024 at 10:08:57PM +0000, Usman Akinyemi via GitGitGadget wrote:
> @@ -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) || !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;
>  		}

Two last nits from my side, sorry that I didn't spot these earlier:

  - The second line is indented incorrectly. When you have a multi-line
    condition, subsequent lines should align with the opening brace like
    this:

	if (something_something ||
	    something_else)
		frobnicate();

  - The braces around `(strtol_i() || !ctx->uidvalidity)` are a bit
    confusing and unnecessary.

Other than that I'm happy with this series, thanks!

Patrick

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

[Usman Akinyemi via GitGitGadget]

Changes from Version 4:

 * Fix incorrect indentation and remove unnecessary braces to avoid
   confusion.

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              | 12 ++++++++----
 imap-send.c           | 13 ++++++++-----
 merge-ll.c            | 11 +++++++++--
 t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++
 t/t6406-merge-attr.sh |  6 ++++++
 5 files changed, 57 insertions(+), 11 deletions(-)


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

Range-diff vs v4:

 1:  d9c997d7a9c = 1:  d9c997d7a9c daemon: replace atoi() with strtoul_ui() and strtol_i()
 2:  da9ea10e4e1 = 2:  da9ea10e4e1 merge: replace atoi() with strtol_i() for marker size validation
 3:  8982dca646d ! 3:  9b2b2dc8fc8 imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing
     @@ 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) || !ctx->uidvalidity) ||
     -+			!(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) || !cb->ctx)) {
     ++		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;
       		}

-- 
gitgitgadget

[PATCH v5 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
Update tests to ensure these arguments are properly validated.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 daemon.c              | 12 ++++++++----
 t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index cb946e3c95f..a40e435c637 100644
--- a/daemon.c
+++ b/daemon.c
@@ -4,6 +4,7 @@
 #include "abspath.h"
 #include "config.h"
 #include "environment.h"
+#include "gettext.h"
 #include "path.h"
 #include "pkt-line.h"
 #include "protocol.h"
@@ -1308,17 +1309,20 @@ int cmd_main(int argc, const char **argv)
 			continue;
 		}
 		if (skip_prefix(arg, "--timeout=", &v)) {
-			timeout = atoi(v);
+			if (strtoul_ui(v, 10, &timeout))
+				die(_("invalid timeout '%s', expecting a non-negative integer"), v);
 			continue;
 		}
 		if (skip_prefix(arg, "--init-timeout=", &v)) {
-			init_timeout = atoi(v);
+			if (strtoul_ui(v, 10, &init_timeout))
+				die(_("invalid init-timeout '%s', expecting a non-negative integer"), v);
 			continue;
 		}
 		if (skip_prefix(arg, "--max-connections=", &v)) {
-			max_connections = atoi(v);
+			if (strtol_i(v, 10, &max_connections))
+				die(_("invalid max-connections '%s', expecting an integer"), v);
 			if (max_connections < 0)
-				max_connections = 0;	        /* unlimited */
+				max_connections = 0;  /* unlimited */
 			continue;
 		}
 		if (!strcmp(arg, "--strict-paths")) {
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index c5f08b67996..722ddb8b7fa 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -8,6 +8,32 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
+
+test_expect_success 'daemon rejects invalid --init-timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --init-timeout="$arg" 2>actual_error &&
+		test_write_lines "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
+		test_cmp actual_error expected || return 1
+	done
+'
+
+test_expect_success 'daemon rejects invalid --timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --timeout="$arg" 2>actual_error &&
+		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 ${SQ}$arg${SQ}, expecting an integer" >expected &&
+	test_cmp actual_error expected
+'
+
 start_git_daemon
 
 check_verbose_connect () {
-- 
gitgitgadget

[PATCH v5 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
Update the test to verify invalid input handling.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 merge-ll.c            | 11 +++++++++--
 t/t6406-merge-attr.sh |  6 ++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 8e63071922b..62fc625552d 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -15,6 +15,7 @@
 #include "merge-ll.h"
 #include "quote.h"
 #include "strbuf.h"
+#include "gettext.h"
 
 struct ll_merge_driver;
 
@@ -427,7 +428,10 @@ 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)) {
+			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;
 	}
@@ -454,7 +458,10 @@ 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 (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;
 	}
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 9bf95249347..c2a9cf03808 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -118,6 +118,12 @@ test_expect_success 'retry the merge with longer context' '
 	grep "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<" actual
 '
 
+test_expect_success 'invalid conflict-marker-size 3a' '
+    echo "text conflict-marker-size=3a" >>.gitattributes &&
+    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' '
 
 	echo "* merge=union" >.gitattributes &&
-- 
gitgitgadget

[PATCH v5 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
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 <usmanakinyemi202@gmail.com>
---
 imap-send.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index ec68a066877..488c06e6139 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) || !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) || !imap->uidnext) {
 			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) || !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;
 		}
@@ -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)) {
+				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;
-- 
gitgitgadget

Re: [PATCH v4 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi]

On Wed, Oct 23, 2024 at 6:05 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Oct 22, 2024 at 10:08:57PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > @@ -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) || !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;
> >               }
>
> Two last nits from my side, sorry that I didn't spot these earlier:
>
>   - The second line is indented incorrectly. When you have a multi-line
>     condition, subsequent lines should align with the opening brace like
>     this:
>
>         if (something_something ||
>             something_else)
>                 frobnicate();
>
>   - The braces around `(strtol_i() || !ctx->uidvalidity)` are a bit
>     confusing and unnecessary.
Thank you Patrick for bringing my attention to this.
I fixed it now.
Usman
>
> Other than that I'm happy with this series, thanks!
>
> Patrick

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

[Patrick Steinhardt]

On Wed, Oct 23, 2024 at 07:40:17AM +0000, Usman Akinyemi via GitGitGadget wrote:
> Changes from Version 4:
> 
>  * Fix incorrect indentation and remove unnecessary braces to avoid
>    confusion.

Thanks, this version looks good to me!

Patrick

Re: [PATCH v5 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Taylor Blau]

On Wed, Oct 23, 2024 at 07:40:18AM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>
> 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.
> Update tests to ensure these arguments are properly validated.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>  daemon.c              | 12 ++++++++----
>  t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index cb946e3c95f..a40e435c637 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -4,6 +4,7 @@
>  #include "abspath.h"
>  #include "config.h"
>  #include "environment.h"
> +#include "gettext.h"
>  #include "path.h"
>  #include "pkt-line.h"
>  #include "protocol.h"
> @@ -1308,17 +1309,20 @@ int cmd_main(int argc, const char **argv)
>  			continue;
>  		}
>  		if (skip_prefix(arg, "--timeout=", &v)) {
> -			timeout = atoi(v);
> +			if (strtoul_ui(v, 10, &timeout))
> +				die(_("invalid timeout '%s', expecting a non-negative integer"), v);
>  			continue;
>  		}
>  		if (skip_prefix(arg, "--init-timeout=", &v)) {
> -			init_timeout = atoi(v);
> +			if (strtoul_ui(v, 10, &init_timeout))
> +				die(_("invalid init-timeout '%s', expecting a non-negative integer"), v);
>  			continue;
>  		}
>  		if (skip_prefix(arg, "--max-connections=", &v)) {
> -			max_connections = atoi(v);
> +			if (strtol_i(v, 10, &max_connections))
> +				die(_("invalid max-connections '%s', expecting an integer"), v);
>  			if (max_connections < 0)
> -				max_connections = 0;	        /* unlimited */
> +				max_connections = 0;  /* unlimited */
>  			continue;
>  		}
>  		if (!strcmp(arg, "--strict-paths")) {
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index c5f08b67996..722ddb8b7fa 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -8,6 +8,32 @@ TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  . "$TEST_DIRECTORY"/lib-git-daemon.sh
> +
> +test_expect_success 'daemon rejects invalid --init-timeout values' '
> +	for arg in "3a" "-3"
> +	do
> +		test_must_fail git daemon --init-timeout="$arg" 2>actual_error &&
> +		test_write_lines "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&

Hmmph. test_write_lines is typically called like

    test_write_lines 1 2 3

to write a file which contains three lines where each of the arguments
appears on its own line.

But here you pass a single argument, which causes us to write out a
single line. Is there a reason for this? If not, I would expect us to
write:

    echo "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expect

Or, perhaps even cleaner would be to do:

    test_must_fail git daemon --init-timeout="$arg" 2>err &&
    test_grep "invalid init-timeout ${SQ}$arg${SQ}" err

since I don't think asserting on the actual error contents matching
verbatim what we expect is adding all that much.

(Also throughout you write 2>actual_err, but redirecting 2>err is more
concise and in convention with the rest of the test suite's style).

> +		test_cmp actual_error expected || return 1
> +	done
> +'
> +
> +test_expect_success 'daemon rejects invalid --timeout values' '
> +	for arg in "3a" "-3"
> +	do
> +		test_must_fail git daemon --timeout="$arg" 2>actual_error &&
> +		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 ${SQ}$arg${SQ}, expecting an integer" >expected &&
> +	test_cmp actual_error expected
> +'
> +

Same notes from above in these two as well.

Thanks,
Taylor

Re: [PATCH v5 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Taylor Blau]

On Wed, Oct 23, 2024 at 07:40:19AM +0000, Usman Akinyemi via GitGitGadget wrote:
> diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
> index 9bf95249347..c2a9cf03808 100755
> --- a/t/t6406-merge-attr.sh
> +++ b/t/t6406-merge-attr.sh
> @@ -118,6 +118,12 @@ test_expect_success 'retry the merge with longer context' '
>  	grep "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<" actual
>  '
>
> +test_expect_success 'invalid conflict-marker-size 3a' '
> +    echo "text conflict-marker-size=3a" >>.gitattributes &&
> +    git checkout -m text 2>error &&
> +    test_grep "warning: invalid marker-size ${SQ}3a${SQ}, expecting an integer" error
> +'
> +

Do subsequent tests further down in this script depend on .gitattributes
not having invalid lines? If so, you may want to instead write:

    cp .gitattributes .gitattributes.bak &&
    echo "text conflict-marker-size=3a" >>.gitattributes &&
    test_when_finished "mv .gitattributes.bak .gitattributes" &&

instead.

Thanks,
Taylor

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

[Taylor Blau]

On Wed, Oct 23, 2024 at 10:52:36AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 23, 2024 at 07:40:17AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > Changes from Version 4:
> >
> >  * Fix incorrect indentation and remove unnecessary braces to avoid
> >    confusion.
>
> Thanks, this version looks good to me!

Thanks for reviewing. This one is looking pretty close, and I've moved
it to 'jch' in my tree, but I think there are still a few lingering
comments that I'd like to see addressed in a subsequent round before we
start merging this one down.

Thanks,
Taylor

Re: [PATCH v5 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Usman Akinyemi]

On Wed, Oct 23, 2024 at 8:31 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Wed, Oct 23, 2024 at 07:40:18AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> >
> > 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.
> > Update tests to ensure these arguments are properly validated.
> >
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
> >  daemon.c              | 12 ++++++++----
> >  t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++
> >  2 files changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/daemon.c b/daemon.c
> > index cb946e3c95f..a40e435c637 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -4,6 +4,7 @@
> >  #include "abspath.h"
> >  #include "config.h"
> >  #include "environment.h"
> > +#include "gettext.h"
> >  #include "path.h"
> >  #include "pkt-line.h"
> >  #include "protocol.h"
> > @@ -1308,17 +1309,20 @@ int cmd_main(int argc, const char **argv)
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--timeout=", &v)) {
> > -                     timeout = atoi(v);
> > +                     if (strtoul_ui(v, 10, &timeout))
> > +                             die(_("invalid timeout '%s', expecting a non-negative integer"), v);
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--init-timeout=", &v)) {
> > -                     init_timeout = atoi(v);
> > +                     if (strtoul_ui(v, 10, &init_timeout))
> > +                             die(_("invalid init-timeout '%s', expecting a non-negative integer"), v);
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--max-connections=", &v)) {
> > -                     max_connections = atoi(v);
> > +                     if (strtol_i(v, 10, &max_connections))
> > +                             die(_("invalid max-connections '%s', expecting an integer"), v);
> >                       if (max_connections < 0)
> > -                             max_connections = 0;            /* unlimited */
> > +                             max_connections = 0;  /* unlimited */
> >                       continue;
> >               }
> >               if (!strcmp(arg, "--strict-paths")) {
> > diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> > index c5f08b67996..722ddb8b7fa 100755
> > --- a/t/t5570-git-daemon.sh
> > +++ b/t/t5570-git-daemon.sh
> > @@ -8,6 +8,32 @@ TEST_PASSES_SANITIZE_LEAK=true
> >  . ./test-lib.sh
> >
> >  . "$TEST_DIRECTORY"/lib-git-daemon.sh
> > +
> > +test_expect_success 'daemon rejects invalid --init-timeout values' '
> > +     for arg in "3a" "-3"
> > +     do
> > +             test_must_fail git daemon --init-timeout="$arg" 2>actual_error &&
> > +             test_write_lines "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
>
> Hmmph. test_write_lines is typically called like
>
>     test_write_lines 1 2 3
>
> to write a file which contains three lines where each of the arguments
> appears on its own line.
>
> But here you pass a single argument, which causes us to write out a
> single line. Is there a reason for this? If not, I would expect us to
> write:
>
>     echo "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expect
>
> Or, perhaps even cleaner would be to do:
>
>     test_must_fail git daemon --init-timeout="$arg" 2>err &&
>     test_grep "invalid init-timeout ${SQ}$arg${SQ}" err
>
Thanks and noted, I will use this approach.
I will send a new patch to fix this.
Usman
> since I don't think asserting on the actual error contents matching
> verbatim what we expect is adding all that much.
>
> (Also throughout you write 2>actual_err, but redirecting 2>err is more
> concise and in convention with the rest of the test suite's style).
>
> > +             test_cmp actual_error expected || return 1
> > +     done
> > +'
> > +
> > +test_expect_success 'daemon rejects invalid --timeout values' '
> > +     for arg in "3a" "-3"
> > +     do
> > +             test_must_fail git daemon --timeout="$arg" 2>actual_error &&
> > +             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 ${SQ}$arg${SQ}, expecting an integer" >expected &&
> > +     test_cmp actual_error expected
> > +'
> > +
>
> Same notes from above in these two as well.
>
> Thanks,
> Taylor

Re: [PATCH v5 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi]

On Wed, Oct 23, 2024 at 8:32 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Wed, Oct 23, 2024 at 07:40:19AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
> > index 9bf95249347..c2a9cf03808 100755
> > --- a/t/t6406-merge-attr.sh
> > +++ b/t/t6406-merge-attr.sh
> > @@ -118,6 +118,12 @@ test_expect_success 'retry the merge with longer context' '
> >       grep "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<" actual
> >  '
> >
> > +test_expect_success 'invalid conflict-marker-size 3a' '
> > +    echo "text conflict-marker-size=3a" >>.gitattributes &&
> > +    git checkout -m text 2>error &&
> > +    test_grep "warning: invalid marker-size ${SQ}3a${SQ}, expecting an integer" error
> > +'
> > +
>
> Do subsequent tests further down in this script depend on .gitattributes
> not having invalid lines? If so, you may want to instead write:
>
>     cp .gitattributes .gitattributes.bak &&
>     echo "text conflict-marker-size=3a" >>.gitattributes &&
>     test_when_finished "mv .gitattributes.bak .gitattributes" &&
>
> instead.
Thanks for the review.
Usman
>
> Thanks,
> Taylor

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

[Usman Akinyemi via GitGitGadget]

Changes from Version 5:

 * Save the content of .gitattributes in .gitattributes.bak before adding
   conflict-marker-size=3a, as subsequent tests do not depend on having
   invalid lines. Restore .gitattributes to its original state after
   testing.
 * Use test_grep for testing failure output, as it provides a cleaner
   approach.
 * Use err instead of actual_error for conciseness and to maintain
   consistency with the style of the rest of the test suite.

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              | 12 ++++++++----
 imap-send.c           | 13 ++++++++-----
 merge-ll.c            | 11 +++++++++--
 t/t5570-git-daemon.sh | 25 +++++++++++++++++++++++++
 t/t6406-merge-attr.sh |  8 ++++++++
 5 files changed, 58 insertions(+), 11 deletions(-)


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

Range-diff vs v5:

 1:  d9c997d7a9c ! 1:  3daedaeb260 daemon: replace atoi() with strtoul_ui() and strtol_i()
     @@ t/t5570-git-daemon.sh: TEST_PASSES_SANITIZE_LEAK=true
      +test_expect_success 'daemon rejects invalid --init-timeout values' '
      +	for arg in "3a" "-3"
      +	do
     -+		test_must_fail git daemon --init-timeout="$arg" 2>actual_error &&
     -+		test_write_lines "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
     -+		test_cmp actual_error expected || return 1
     ++		test_must_fail git daemon --init-timeout="$arg" 2>err &&
     ++		test_grep "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" err ||
     ++		return 1
      +	done
      +'
      +
      +test_expect_success 'daemon rejects invalid --timeout values' '
      +	for arg in "3a" "-3"
      +	do
     -+		test_must_fail git daemon --timeout="$arg" 2>actual_error &&
     -+		test_write_lines "fatal: invalid timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
     -+		test_cmp actual_error expected || return 1
     ++		test_must_fail git daemon --timeout="$arg" 2>err &&
     ++		test_grep "fatal: invalid timeout ${SQ}$arg${SQ}, expecting a non-negative integer" err ||
     ++		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 ${SQ}$arg${SQ}, expecting an integer" >expected &&
     -+	test_cmp actual_error expected
     ++	test_must_fail git daemon --max-connections=3a 2>err &&
     ++	test_grep "fatal: invalid max-connections ${SQ}$arg${SQ}, expecting an integer" err
      +'
      +
       start_git_daemon
 2:  da9ea10e4e1 ! 2:  0ea3b349560 merge: replace atoi() with strtol_i() for marker size validation
     @@ 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 &&
     -+    git checkout -m text 2>error &&
     -+    test_grep "warning: invalid marker-size ${SQ}3a${SQ}, expecting an integer" error
     ++	cp .gitattributes .gitattributes.bak &&
     ++	echo "text conflict-marker-size=3a" >>.gitattributes &&
     ++	test_when_finished "mv .gitattributes.bak .gitattributes" &&
     ++	git checkout -m text 2>err &&
     ++	test_grep "warning: invalid marker-size ${SQ}3a${SQ}, expecting an integer" err
      +'
      +
       test_expect_success 'custom merge backend' '
 3:  9b2b2dc8fc8 = 3:  17484df5200 imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

-- 
gitgitgadget

[PATCH v6 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
Update tests to ensure these arguments are properly validated.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 daemon.c              | 12 ++++++++----
 t/t5570-git-daemon.sh | 25 +++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index cb946e3c95f..a40e435c637 100644
--- a/daemon.c
+++ b/daemon.c
@@ -4,6 +4,7 @@
 #include "abspath.h"
 #include "config.h"
 #include "environment.h"
+#include "gettext.h"
 #include "path.h"
 #include "pkt-line.h"
 #include "protocol.h"
@@ -1308,17 +1309,20 @@ int cmd_main(int argc, const char **argv)
 			continue;
 		}
 		if (skip_prefix(arg, "--timeout=", &v)) {
-			timeout = atoi(v);
+			if (strtoul_ui(v, 10, &timeout))
+				die(_("invalid timeout '%s', expecting a non-negative integer"), v);
 			continue;
 		}
 		if (skip_prefix(arg, "--init-timeout=", &v)) {
-			init_timeout = atoi(v);
+			if (strtoul_ui(v, 10, &init_timeout))
+				die(_("invalid init-timeout '%s', expecting a non-negative integer"), v);
 			continue;
 		}
 		if (skip_prefix(arg, "--max-connections=", &v)) {
-			max_connections = atoi(v);
+			if (strtol_i(v, 10, &max_connections))
+				die(_("invalid max-connections '%s', expecting an integer"), v);
 			if (max_connections < 0)
-				max_connections = 0;	        /* unlimited */
+				max_connections = 0;  /* unlimited */
 			continue;
 		}
 		if (!strcmp(arg, "--strict-paths")) {
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index c5f08b67996..e3df7d86410 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -8,6 +8,31 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
+
+test_expect_success 'daemon rejects invalid --init-timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --init-timeout="$arg" 2>err &&
+		test_grep "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" err ||
+		return 1
+	done
+'
+
+test_expect_success 'daemon rejects invalid --timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --timeout="$arg" 2>err &&
+		test_grep "fatal: invalid timeout ${SQ}$arg${SQ}, expecting a non-negative integer" err ||
+		return 1
+	done
+'
+
+test_expect_success 'daemon rejects invalid --max-connections values' '
+	arg='3a' &&
+	test_must_fail git daemon --max-connections=3a 2>err &&
+	test_grep "fatal: invalid max-connections ${SQ}$arg${SQ}, expecting an integer" err
+'
+
 start_git_daemon
 
 check_verbose_connect () {
-- 
gitgitgadget

[PATCH v6 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
Update the test to verify invalid input handling.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 merge-ll.c            | 11 +++++++++--
 t/t6406-merge-attr.sh |  8 ++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 8e63071922b..62fc625552d 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -15,6 +15,7 @@
 #include "merge-ll.h"
 #include "quote.h"
 #include "strbuf.h"
+#include "gettext.h"
 
 struct ll_merge_driver;
 
@@ -427,7 +428,10 @@ 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)) {
+			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;
 	}
@@ -454,7 +458,10 @@ 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 (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;
 	}
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 9bf95249347..2dfc9a873d4 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -118,6 +118,14 @@ test_expect_success 'retry the merge with longer context' '
 	grep "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<" actual
 '
 
+test_expect_success 'invalid conflict-marker-size 3a' '
+	cp .gitattributes .gitattributes.bak &&
+	echo "text conflict-marker-size=3a" >>.gitattributes &&
+	test_when_finished "mv .gitattributes.bak .gitattributes" &&
+	git checkout -m text 2>err &&
+	test_grep "warning: invalid marker-size ${SQ}3a${SQ}, expecting an integer" err
+'
+
 test_expect_success 'custom merge backend' '
 
 	echo "* merge=union" >.gitattributes &&
-- 
gitgitgadget

[PATCH v6 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

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.
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 <usmanakinyemi202@gmail.com>
---
 imap-send.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index ec68a066877..488c06e6139 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) || !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) || !imap->uidnext) {
 			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) || !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;
 		}
@@ -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)) {
+				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;
-- 
gitgitgadget

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

[Usman Akinyemi]

On Wed, Oct 23, 2024 at 8:33 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Wed, Oct 23, 2024 at 10:52:36AM +0200, Patrick Steinhardt wrote:
> > On Wed, Oct 23, 2024 at 07:40:17AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > Changes from Version 4:
> > >
> > >  * Fix incorrect indentation and remove unnecessary braces to avoid
> > >    confusion.
> >
> > Thanks, this version looks good to me!
>
> Thanks for reviewing. This one is looking pretty close, and I've moved
> it to 'jch' in my tree, but I think there are still a few lingering
> comments that I'd like to see addressed in a subsequent round before we
> start merging this one down.
>
Thanks to all the mentors, I really appreciate your time. I have sent
another round.
Usman.
> Thanks,
> Taylor

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

[Taylor Blau]

On Thu, Oct 24, 2024 at 12:24:55AM +0000, Usman Akinyemi via GitGitGadget wrote:
> 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

Thanks, this new round looks quite good to me. Do others have thoughts
on this, or are we ready to start merging it down?

Thanks,
Taylor

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

[Patrick Steinhardt]

On Thu, Oct 24, 2024 at 02:03:12PM -0400, Taylor Blau wrote:
> On Thu, Oct 24, 2024 at 12:24:55AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > 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
> 
> Thanks, this new round looks quite good to me. Do others have thoughts
> on this, or are we ready to start merging it down?

I'm happy with this version.

Patrick

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

[Usman Akinyemi]

On Fri, Oct 25, 2024 at 5:07 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Oct 24, 2024 at 02:03:12PM -0400, Taylor Blau wrote:
> > On Thu, Oct 24, 2024 at 12:24:55AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > 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
> >
> > Thanks, this new round looks quite good to me. Do others have thoughts
> > on this, or are we ready to start merging it down?
>
> I'm happy with this version.
>
> Patrick
Thanks to Patrick and Taylor, I really appreciate your time and mentorship.

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

[Taylor Blau]

On Fri, Oct 25, 2024 at 06:11:05AM +0000, Usman Akinyemi wrote:
> On Fri, Oct 25, 2024 at 5:07 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Thu, Oct 24, 2024 at 02:03:12PM -0400, Taylor Blau wrote:
> > > On Thu, Oct 24, 2024 at 12:24:55AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > 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
> > >
> > > Thanks, this new round looks quite good to me. Do others have thoughts
> > > on this, or are we ready to start merging it down?
> >
> > I'm happy with this version.
> >
> > Patrick
>
> Thanks to Patrick and Taylor, I really appreciate your time and mentorship.

Thanks, both, for working on and reviewing this topic.

Thanks,
Taylor

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Phillip Wood]

Hi Patrick and Usman

On 21/10/2024 13:20, Patrick Steinhardt wrote:
> On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> These are a bit curious. As your test demonstrates, we retrieve the
> values from the "gitattributes" file. And given that the file tends to be
> checked into the repository, you can now basically break somebody elses
> commands by having an invalid value in there.
> 
> That makes me think that we likely shouldn't die here. We may print a
> warning, but other than that we should likely continue and use the
> DEFAULT_CONFLICT_MARKER_SIZE.

I think using a warning here is a good idea, we should probably fix the 
whitespace attributes to do the same. If you have

     * whitespace=indent-with-non-tab,tab-in-indent

in .gitattributes then "git diff" dies with

     fatal: cannot enforce both tab-in-indent and indent-with-non-tab

Anyway that's not really related to this series but I thought I'd add it 
as #leftoverbits for future reference.

Thanks for working on this Usman, what is queued in next looks good to me.

Best Wishes

Phillip


> Patrick
> 

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi]

On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Patrick and Usman
>
> On 21/10/2024 13:20, Patrick Steinhardt wrote:
> > On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> >> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > These are a bit curious. As your test demonstrates, we retrieve the
> > values from the "gitattributes" file. And given that the file tends to be
> > checked into the repository, you can now basically break somebody elses
> > commands by having an invalid value in there.
> >
> > That makes me think that we likely shouldn't die here. We may print a
> > warning, but other than that we should likely continue and use the
> > DEFAULT_CONFLICT_MARKER_SIZE.
>
> I think using a warning here is a good idea, we should probably fix the
> whitespace attributes to do the same. If you have
>
>      * whitespace=indent-with-non-tab,tab-in-indent
>
> in .gitattributes then "git diff" dies with
>
>      fatal: cannot enforce both tab-in-indent and indent-with-non-tab
>
> Anyway that's not really related to this series but I thought I'd add it
> as #leftoverbits for future reference.
>
> Thanks for working on this Usman, what is queued in next looks good to me.
Hi Philip,

I just checked it. I will be glad to work on it.

I also noticed that the test used for testing used a different
approach(test_must_fail) compared to the one I wrote which used
test_grep. Should I change the test also ?

Also, when should someone redirect a warning/failure into a file then
use test_grep or just used test_must_fail ?

Thank you
Usman Akinyemi
>
> Best Wishes
>
> Phillip
>
>
> > Patrick
> >
>

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Phillip Wood]

Hi Usman

On 30/10/2024 16:19, Usman Akinyemi wrote:
> On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 21/10/2024 13:20, Patrick Steinhardt wrote:
>>> On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
>>>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>>> These are a bit curious. As your test demonstrates, we retrieve the
>>> values from the "gitattributes" file. And given that the file tends to be
>>> checked into the repository, you can now basically break somebody elses
>>> commands by having an invalid value in there.
>>>
>>> That makes me think that we likely shouldn't die here. We may print a
>>> warning, but other than that we should likely continue and use the
>>> DEFAULT_CONFLICT_MARKER_SIZE.
>>
>> I think using a warning here is a good idea, we should probably fix the
>> whitespace attributes to do the same. If you have
>>
>>       * whitespace=indent-with-non-tab,tab-in-indent
>>
>> in .gitattributes then "git diff" dies with
>>
>>       fatal: cannot enforce both tab-in-indent and indent-with-non-tab
>>
>> Anyway that's not really related to this series but I thought I'd add it
>> as #leftoverbits for future reference.
>>
>> Thanks for working on this Usman, what is queued in next looks good to me.
> 
> I just checked it. I will be glad to work on it.

If you want to work on this that's great, but please don't feel any 
obligation to do so.

> I also noticed that the test used for testing used a different
> approach(test_must_fail) compared to the one I wrote which used
> test_grep. Should I change the test also ?

I'm not sure which test you are looking at but I assume it is using 
test_must_fail because the command being tested is expected to die. If 
we change the code to print a warning instead then we'd need to capture 
stderr and use test_grep or test_cmp. Note that we only want to print a 
warning when parsing .gitattributes, the other callers of 
parse_whitespace_rule() still want to die. Also we should decide what 
value to use when the user provides both - neither indent-with-non-tab 
or tab-in-indent are on by default so it's not clear exactly what we 
should do.

> Also, when should someone redirect a warning/failure into a file then
> use test_grep or just used test_must_fail ?

You must use test_must_fail if you expect a git command to fail, if you 
expect the command to print a warning but exit successfully you should 
not use test_must_fail. So if you expect a command to fail and print an 
error or warning then you'd do

     test_must_fail git my failing command 2>err &&
     test_grep "error message" err

test_must_fail checks that the command fails, but reports an error if 
the command is killed by a signal such as SIGSEV.

Best Wishes

Phillip

> Thank you
> Usman Akinyemi
>>
>> Best Wishes
>>
>> Phillip
>>
>>
>>> Patrick
>>>
>>
> 

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi]

On Thu, Oct 31, 2024 at 9:58 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Usman
>
> On 30/10/2024 16:19, Usman Akinyemi wrote:
> > On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >> On 21/10/2024 13:20, Patrick Steinhardt wrote:
> >>> On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> >>>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> >>> These are a bit curious. As your test demonstrates, we retrieve the
> >>> values from the "gitattributes" file. And given that the file tends to be
> >>> checked into the repository, you can now basically break somebody elses
> >>> commands by having an invalid value in there.
> >>>
> >>> That makes me think that we likely shouldn't die here. We may print a
> >>> warning, but other than that we should likely continue and use the
> >>> DEFAULT_CONFLICT_MARKER_SIZE.
> >>
> >> I think using a warning here is a good idea, we should probably fix the
> >> whitespace attributes to do the same. If you have
> >>
> >>       * whitespace=indent-with-non-tab,tab-in-indent
> >>
> >> in .gitattributes then "git diff" dies with
> >>
> >>       fatal: cannot enforce both tab-in-indent and indent-with-non-tab
> >>
> >> Anyway that's not really related to this series but I thought I'd add it
> >> as #leftoverbits for future reference.
> >>
> >> Thanks for working on this Usman, what is queued in next looks good to me.
> >
> > I just checked it. I will be glad to work on it.
>
> If you want to work on this that's great, but please don't feel any
> obligation to do so.
>
> > I also noticed that the test used for testing used a different
> > approach(test_must_fail) compared to the one I wrote which used
> > test_grep. Should I change the test also ?
>
> I'm not sure which test you are looking at but I assume it is using
> test_must_fail because the command being tested is expected to die. If
> we change the code to print a warning instead then we'd need to capture
> stderr and use test_grep or test_cmp. Note that we only want to print a
> warning when parsing .gitattributes, the other callers of
> parse_whitespace_rule() still want to die. Also we should decide what
> value to use when the user provides both - neither indent-with-non-tab
> or tab-in-indent are on by default so it's not clear exactly what we
> should do.
Hi Philip,

I understand, we will have to pick one if we are to use a warning in this case,
indent-with-non-tab seems to be a good candidate as it is not excluded
by default.

We can have something like this

    if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {
        warning(_("cannot enforce both tab-in-indent and
indent-with-non-tab, removing tab-in-indent"));
        rule &= ~WS_TAB_IN_INDENT;
    }
and this for default
#define WS_DEFAULT_RULE (WS_TRAILING_SPACE | WS_SPACE_BEFORE_TAB |
WS_INDENT_WITH_NON_TAB | 8)

or just leave the WS_DEFAULT_RULE as it is and remove WS_TAB_IN_INDENT
in case both are set.

what do you think ?

Thank you.
Usman


>
> > Also, when should someone redirect a warning/failure into a file then
> > use test_grep or just used test_must_fail ?
>
> You must use test_must_fail if you expect a git command to fail, if you
> expect the command to print a warning but exit successfully you should
> not use test_must_fail. So if you expect a command to fail and print an
> error or warning then you'd do
>
>      test_must_fail git my failing command 2>err &&
>      test_grep "error message" err
>
> test_must_fail checks that the command fails, but reports an error if
> the command is killed by a signal such as SIGSEV.
Thanks for the explanation. I understand it well now.
>
> Best Wishes
>
> Phillip
>
> > Thank you
> > Usman Akinyemi
> >>
> >> Best Wishes
> >>
> >> Phillip
> >>
> >>
> >>> Patrick
> >>>
> >>
> >
>

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[Usman Akinyemi]

On Thu, Oct 31, 2024 at 12:21 PM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
>
> On Thu, Oct 31, 2024 at 9:58 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> > Hi Usman
> >
> > On 30/10/2024 16:19, Usman Akinyemi wrote:
> > > On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > >> On 21/10/2024 13:20, Patrick Steinhardt wrote:
> > >>> On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > >>>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > >>> These are a bit curious. As your test demonstrates, we retrieve the
> > >>> values from the "gitattributes" file. And given that the file tends to be
> > >>> checked into the repository, you can now basically break somebody elses
> > >>> commands by having an invalid value in there.
> > >>>
> > >>> That makes me think that we likely shouldn't die here. We may print a
> > >>> warning, but other than that we should likely continue and use the
> > >>> DEFAULT_CONFLICT_MARKER_SIZE.
> > >>
> > >> I think using a warning here is a good idea, we should probably fix the
> > >> whitespace attributes to do the same. If you have
> > >>
> > >>       * whitespace=indent-with-non-tab,tab-in-indent
> > >>
> > >> in .gitattributes then "git diff" dies with
> > >>
> > >>       fatal: cannot enforce both tab-in-indent and indent-with-non-tab
> > >>
> > >> Anyway that's not really related to this series but I thought I'd add it
> > >> as #leftoverbits for future reference.
> > >>
> > >> Thanks for working on this Usman, what is queued in next looks good to me.
> > >
> > > I just checked it. I will be glad to work on it.
> >
> > If you want to work on this that's great, but please don't feel any
> > obligation to do so.
> >
> > > I also noticed that the test used for testing used a different
> > > approach(test_must_fail) compared to the one I wrote which used
> > > test_grep. Should I change the test also ?
> >
> > I'm not sure which test you are looking at but I assume it is using
> > test_must_fail because the command being tested is expected to die. If
> > we change the code to print a warning instead then we'd need to capture
> > stderr and use test_grep or test_cmp. Note that we only want to print a
> > warning when parsing .gitattributes, the other callers of
> > parse_whitespace_rule() still want to die. Also we should decide what
> > value to use when the user provides both - neither indent-with-non-tab
> > or tab-in-indent are on by default so it's not clear exactly what we
> > should do.
> Hi Philip,
>
> I understand, we will have to pick one if we are to use a warning in this case,
> indent-with-non-tab seems to be a good candidate as it is not excluded
> by default.
>
> We can have something like this
>
>     if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {
>         warning(_("cannot enforce both tab-in-indent and
> indent-with-non-tab, removing tab-in-indent"));
>         rule &= ~WS_TAB_IN_INDENT;
>     }
> and this for default
> #define WS_DEFAULT_RULE (WS_TRAILING_SPACE | WS_SPACE_BEFORE_TAB |
> WS_INDENT_WITH_NON_TAB | 8)
>
> or just leave the WS_DEFAULT_RULE as it is and remove WS_TAB_IN_INDENT
> in case both are set.
>
> what do you think ?
>
> Thank you.
> Usman
Hello,

Bringing attention to this.
>
>
> >
> > > Also, when should someone redirect a warning/failure into a file then
> > > use test_grep or just used test_must_fail ?
> >
> > You must use test_must_fail if you expect a git command to fail, if you
> > expect the command to print a warning but exit successfully you should
> > not use test_must_fail. So if you expect a command to fail and print an
> > error or warning then you'd do
> >
> >      test_must_fail git my failing command 2>err &&
> >      test_grep "error message" err
> >
> > test_must_fail checks that the command fails, but reports an error if
> > the command is killed by a signal such as SIGSEV.
> Thanks for the explanation. I understand it well now.
> >
> > Best Wishes
> >
> > Phillip
> >
> > > Thank you
> > > Usman Akinyemi
> > >>
> > >> Best Wishes
> > >>
> > >> Phillip
> > >>
> > >>
> > >>> Patrick
> > >>>
> > >>
> > >
> >

Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[phillip.wood123]

Hi Usman

Sorry for the slow response

On 31/10/2024 12:21, Usman Akinyemi wrote:
> On Thu, Oct 31, 2024 at 9:58 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 30/10/2024 16:19, Usman Akinyemi wrote:
>>
>> If you want to work on this that's great, but please don't feel any
>> obligation to do so.
>>
>>> I also noticed that the test used for testing used a different
>>> approach(test_must_fail) compared to the one I wrote which used
>>> test_grep. Should I change the test also ?
>>
>> I'm not sure which test you are looking at but I assume it is using
>> test_must_fail because the command being tested is expected to die. If
>> we change the code to print a warning instead then we'd need to capture
>> stderr and use test_grep or test_cmp. Note that we only want to print a
>> warning when parsing .gitattributes, the other callers of
>> parse_whitespace_rule() still want to die. Also we should decide what
>> value to use when the user provides both - neither indent-with-non-tab
>> or tab-in-indent are on by default so it's not clear exactly what we
>> should do.
> Hi Philip,
> 
> I understand, we will have to pick one if we are to use a warning in this case,
> indent-with-non-tab seems to be a good candidate as it is not excluded
> by default.

I'm not sure I understand I what you mean by "not excluded by default".

 > We can have something like this>
>      if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {
>          warning(_("cannot enforce both tab-in-indent and
> indent-with-non-tab, removing tab-in-indent"));
>          rule &= ~WS_TAB_IN_INDENT;
>      }

That sounds reasonable for the cases where we want to warn rather than die.

> and this for default
> #define WS_DEFAULT_RULE (WS_TRAILING_SPACE | WS_SPACE_BEFORE_TAB |
> WS_INDENT_WITH_NON_TAB | 8)
> 
> or just leave the WS_DEFAULT_RULE as it is and remove WS_TAB_IN_INDENT
> in case both are set.

I don't think we want to change the default rule as it could cause 
problems for users who rely on it.

Best Wishes

Phillip