R atoi

From: Usman Akinyemi <[email protected]>

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 <[email protected]>

---
 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

t/t3404-rebase-interactive.sh +0 -0

index f171af3061d..96a65783c47 100755

From: Usman Akinyemi <[email protected]>

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 <[email protected]>

---
 t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

t/t3404-rebase-interactive.sh +0 -0

index 96a65783c47..2ab660ef30f 100755

From: Usman Akinyemi <[email protected]>

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 <[email protected]>

Cc [email protected]

Cc Patrick Steinhardt <[email protected]>

Cc [email protected]

Cc Christian Couder <[email protected]>

Cc Eric Sunshine <[email protected]>

Cc Taylor Blau <[email protected]>

---
 daemon.c    | 14 +++++++++-----
 imap-send.c | 13 ++++++++-----
 merge-ll.c  |  6 ++----
 3 files changed, 19 insertions(+), 14 deletions(-)

daemon.c +9 -5

index cb946e3c95f..3fdb6e83c40 100644
@@ -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")) {

imap-send.c +8 -5

index ec68a066877..33b74dfded7 100644
@@ -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;

merge-ll.c +2 -5

index 8e63071922b..2bfee0f2c6b 100644
@@ -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

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.

Hi Usman

Quoted 5 lines · Usman Akinyemi

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

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

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

Quoted 7 lines

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/

Quoted 8 lines

Signed-off-by Usman Akinyemi <[email protected]>

Cc [email protected]

Cc Patrick Steinhardt <[email protected]>

Cc [email protected]

Cc Christian Couder <[email protected]>

Cc Eric Sunshine <[email protected]>

Cc Taylor Blau <[email protected]>

Quoted 6 lines

---
 daemon.c    | 14 +++++++++-----
 imap-send.c | 13 ++++++++-----
 merge-ll.c  |  6 ++----
 3 files changed, 19 insertions(+), 14 deletions(-)

Quoted 5 lines

daemon.c +0 -0

index cb946e3c95f..3fdb6e83c40 100644

daemon.c int cmd_main(int argc, const char **argv) · line 1315

Quoted 9 lines

daemon.c +3 -1

@@ -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;
                }

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.

Quoted 9 lines · Usman Akinyemi

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

Quoted 7 lines

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.

Quoted 7 lines

Quoted 7 lines

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/

Quoted 8 lines

Quoted 8 lines

Signed-off-by Usman Akinyemi <[email protected]>

Cc [email protected]

Cc Patrick Steinhardt <[email protected]>

Cc [email protected]

Cc Christian Couder <[email protected]>

Cc Eric Sunshine <[email protected]>

Cc Taylor Blau <[email protected]>

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.

Quoted 6 lines

Quoted 6 lines

---
  daemon.c    | 14 +++++++++-----
  imap-send.c | 13 ++++++++-----
  merge-ll.c  |  6 ++----
  3 files changed, 19 insertions(+), 14 deletions(-)

Quoted 14 lines

Quoted 5 lines

daemon.c +0 -0

index cb946e3c95f..3fdb6e83c40 100644

daemon.c int cmd_main(int argc, const char **argv) · line 1315

Quoted 9 lines

daemon.c +3 -1

@@ -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]

Quoted 3 lines

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

Quoted 18 lines · Patrick Steinhardt

Quoted 9 lines · Usman Akinyemi

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 23 lines · Phillip Wood

Quoted 18 lines · Patrick Steinhardt

Quoted 9 lines · Usman Akinyemi

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 6 lines

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

Quoted 31 lines · Patrick Steinhardt

Quoted 23 lines · Phillip Wood

Quoted 18 lines · Patrick Steinhardt

Quoted 9 lines · Usman Akinyemi

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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

Quoted 2 lines

Patrick

Quoted 52 lines · Phillip Wood

Quoted 31 lines · Patrick Steinhardt

Quoted 23 lines · Phillip Wood

Quoted 18 lines · Patrick Steinhardt

Quoted 9 lines · Usman Akinyemi

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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

Quoted 3 lines

Quoted 2 lines

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.

On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi

Quoted 4 lines · Phillip Wood

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 ?

Quoted 1 line

Thanks.

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

Quoted 5 lines · Usman Akinyemi

On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi

Quoted 4 lines · Phillip Wood

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.

Quoted 3 lines

Quoted 1 line

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

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

Quoted 2 lines · Usman Akinyemi

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

Quoted 8 lines · On Mon, Oct 14, 2024 at 6:36 PM <[email protected]> wrote:

Quoted 5 lines · Usman Akinyemi

On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi

Quoted 4 lines · Phillip Wood

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 ?

Quoted 10 lines

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

Quoted 13 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 4 lines · Usman Akinyemi

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"

Quoted 9 lines · On 15/10/2024 19:28, [email protected] wrote:

Quoted 4 lines · Usman Akinyemi

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

Quoted 5 lines · Usman Akinyemi

On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi

Quoted 4 lines · Phillip Wood

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 8 lines · On Mon, Oct 14, 2024 at 6:36 PM <[email protected]> wrote:

Quoted 5 lines · Usman Akinyemi

On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi

Quoted 4 lines · Phillip Wood

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 ?

Quoted 9 lines

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

(quoted diff) +2 -1

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

(quoted diff) +2 -1

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

(quoted diff) +2 -1

-                       max_connections = atoi(v);
+                       if (strtol_i(v, 10, &max_connections))
+                               die("invalid '--max-connections' '%s',

expecting an integer", v);
if (max_connections < 0)

(quoted diff) +1 -1

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

Quoted 50 lines · Usman Akinyemi

Quoted 8 lines · On Mon, Oct 14, 2024 at 6:36 PM <[email protected]> wrote:

Quoted 5 lines · Usman Akinyemi

On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi

Quoted 4 lines · Phillip Wood

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 ?

Quoted 9 lines

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

(quoted diff) +2 -1

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

(quoted diff) +2 -1

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

(quoted diff) +2 -1

-                       max_connections = atoi(v);
+                       if (strtol_i(v, 10, &max_connections))
+                               die("invalid '--max-connections' '%s',

expecting an integer", v);
if (max_connections < 0)

(quoted diff) +1 -1

-                               max_connections = 0;            /* unlimited */
+                               max_connections = 0;  /* unlimited */
                        continue;
                }

Quoted 6 lines

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

Quoted 9 lines · Usman Akinyemi

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

Quoted 7 lines

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.

Quoted 7 lines

Quoted 7 lines

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/

Quoted 8 lines

Quoted 8 lines

Signed-off-by Usman Akinyemi <[email protected]>

Cc [email protected]

Cc Patrick Steinhardt <[email protected]>

Cc [email protected]

Cc Christian Couder <[email protected]>

Cc Eric Sunshine <[email protected]>

Cc Taylor Blau <[email protected]>

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.

Quoted 18 lines

Quoted 18 lines

---
 daemon.c    | 14 +++++++++-----
 imap-send.c | 13 ++++++++-----
 merge-ll.c  |  6 ++----
 3 files changed, 19 insertions(+), 14 deletions(-)

daemon.c +2 -1

index cb946e3c95f..3fdb6e83c40 100644
@@ -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);

Quoted 3 lines

daemon.c int cmd_main(int argc, const char **argv) · line 1315

Quoted 3 lines

daemon.c +1 -0

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

Quoted 3 lines

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

From: Usman Akinyemi <[email protected]>

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 <[email protected]>

Cc [email protected]

Cc Patrick Steinhardt <[email protected]>

Cc [email protected]

Cc Christian Couder <[email protected]>

Cc Eric Sunshine <[email protected]>

Cc Taylor Blau <[email protected]>

---
  daemon.c    | 14 +++++++++-----
  imap-send.c | 13 ++++++++-----
  merge-ll.c  |  6 ++----
  3 files changed, 19 insertions(+), 14 deletions(-)

daemon.c +0 -0

index cb946e3c95f..3fdb6e83c40 100644

daemon.c +2 -0

+				max_connections = 0;  /* unlimited */
+			}
  			continue;
  		}
  		if (!strcmp(arg, "--strict-paths")) {

imap-send.c +0 -0

index ec68a066877..33b74dfded7 100644

imap-send.c +1 -0

+			}
  			for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
  				if (cmdp->tag == tag)
  					goto gottag;

merge-ll.c +0 -0

index 8e63071922b..2bfee0f2c6b 100644

Quoted 6 lines

Cc [email protected]

Cc Patrick Steinhardt <[email protected]>

Cc [email protected]

Cc Christian Couder <[email protected]>

Cc Eric Sunshine <[email protected]>

Cc Taylor Blau <[email protected]>

We do not tend to use Cc: footers on this list. Also note that as there is a blank line between the Signed-off-by: line and this paragraph the
Signed-off-by: will be ignored by git-interpret-trailers.

Quoted 7 lines

Quoted 6 lines

Cc [email protected]

Cc Patrick Steinhardt <[email protected]>

Cc [email protected]

Cc Christian Couder <[email protected]>

Cc Eric Sunshine <[email protected]>

Cc Taylor Blau <[email protected]>

Quoted 3 lines

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.

Hi Usman

Quoted 5 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers and strtol_i() for signed integers across multiple files. This change improves error handling and prevents potential integer overflow issues.

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

Quoted 7 lines

The following files were updated:
- daemon.c: Update parsing of --timeout, --init-timeout, and --max-connections
- imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and tags
- merge-ll.c: Enhance parsing of marker size in ll_merge and ll_merge_marker_size

This information is not really needed in the commit message as it is shown in the diff.

Quoted 3 lines

This change allows for better error detection when parsing integer values from command-line arguments and IMAP responses, making the code more robust and secure.

Great

Quoted 11 lines

This is a #leftoverbit discussed here:
https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/

Signed-off-by Usman Akinyemi <[email protected]>

Cc [email protected]

Cc Patrick Steinhardt <[email protected]>

Cc [email protected]

Cc Christian Couder <[email protected]>

Cc Eric Sunshine <[email protected]>

Cc Taylor Blau <[email protected]>

We do not tend to use Cc: footers on this list. Also note that as there is a blank line between the Signed-off-by: line and this paragraph the
Signed-off-by: will be ignored by git-interpret-trailers.

Quoted 11 lines

---
  daemon.c    | 14 +++++++++-----
  imap-send.c | 13 ++++++++-----
  merge-ll.c  |  6 ++----
  3 files changed, 19 insertions(+), 14 deletions(-)

daemon.c +0 -0

index cb946e3c95f..3fdb6e83c40 100644

daemon.c int cmd_main(int argc, const char **argv) · line 1311

Quoted 5 lines

daemon.c +1 -1

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

daemon.c int cmd_main(int argc, const char **argv) · line 1312

Quoted 1 line

daemon.c +1 -0

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

daemon.c int cmd_main(int argc, const char **argv) · line 1318

Quoted 7 lines

daemon.c +3 -1

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

daemon.c int cmd_main(int argc, const char **argv) · line 1323

Quoted 8 lines

daemon.c +2 -3

+                     }
                      continue;
              }
              if (skip_prefix(arg, "--max-connections=", &v)) {
-                     max_connections = atoi(v);
-                     if (max_connections < 0)
-                             max_connections = 0;            /* unlimited */
+                     if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) {

This is a faithful translation but if the aim of this series is to detect errors then I think we want to do something like

        if (strtol_i(v, 10, &max_connections))
                die(...)

if (max_connections < 0)
max_connections = 0; /* unlimited */

Quoted 10 lines

daemon.c +2 -0

+                             max_connections = 0;  /* unlimited */
+                     }
                      continue;
              }
              if (!strcmp(arg, "--strict-paths")) {

imap-send.c +0 -0

index ec68a066877..33b74dfded7 100644

imap-send.c static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, · line 671

Quoted 5 lines

imap-send.c +1 -1

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

imap-send.c static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, · line 676

Quoted 6 lines

imap-send.c +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

imap-send.c static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, · line 690

Quoted 11 lines

imap-send.c +2 -2

                      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

imap-send.c static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd) · line 776

Quoted 9 lines

imap-send.c +1 -1

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

imap-send.c static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd) · line 778

Quoted 2 lines

imap-send.c +2 -0

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

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

Quoted 9 lines

imap-send.c +1 -0

+                     }
                      for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
                              if (cmdp->tag == tag)
                                      goto gottag;

merge-ll.c +0 -0

index 8e63071922b..2bfee0f2c6b 100644

merge-ll.c enum ll_merge_result ll_merge(mmbuffer_t *result_buf, · line 430

Quoted 6 lines

merge-ll.c +1 -2

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

merge-ll.c int ll_merge_marker_size(struct index_state *istate, const char *path) · line 456

Quoted 10 lines

merge-ll.c +1 -2

                      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

Quoted 12 lines · Phillip Wood

daemon.c int cmd_main(int argc, const char **argv) · line 1323

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

               }
               if (skip_prefix(arg, "--max-connections=", &v)) {

daemon.c +1 -3

-                     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

Quoted 2 lines

         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.

Quoted 110 lines

if (max_connections < 0)
max_connections = 0; /* unlimited */

Quoted 10 lines

daemon.c +2 -0

+                             max_connections = 0;  /* unlimited */
+                     }
                       continue;
               }
               if (!strcmp(arg, "--strict-paths")) {

imap-send.c +0 -0

index ec68a066877..33b74dfded7 100644

imap-send.c static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, · line 671

Quoted 5 lines

imap-send.c +1 -1

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

imap-send.c static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, · line 676

Quoted 6 lines

imap-send.c +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

imap-send.c static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, · line 690

Quoted 11 lines

imap-send.c +2 -2

                       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

imap-send.c static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd) · line 776

Quoted 9 lines

imap-send.c +1 -1

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

imap-send.c static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd) · line 778

Quoted 2 lines

imap-send.c +2 -0

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

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

Quoted 9 lines

imap-send.c +1 -0

+                     }
                       for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
                               if (cmdp->tag == tag)
                                       goto gottag;

merge-ll.c +0 -0

index 8e63071922b..2bfee0f2c6b 100644

merge-ll.c enum ll_merge_result ll_merge(mmbuffer_t *result_buf, · line 430

Quoted 6 lines

merge-ll.c +1 -2

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

merge-ll.c int ll_merge_marker_size(struct index_state *istate, const char *path) · line 456

Quoted 10 lines

merge-ll.c +1 -2

                       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

Hi Usman

Quoted 5 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers and strtol_i() for signed integers across multiple files. This change improves error handling and prevents potential integer overflow issues.

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

Quoted 7 lines

The following files were updated:
- daemon.c: Update parsing of --timeout, --init-timeout, and --max-connections
- imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and tags
- merge-ll.c: Enhance parsing of marker size in ll_merge and ll_merge_marker_size

This information is not really needed in the commit message as it is shown in the diff.

Quoted 3 lines

This change allows for better error detection when parsing integer values from command-line arguments and IMAP responses, making the code more robust and secure.

Great

Quoted 11 lines

This is a #leftoverbit discussed here:
https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/

Signed-off-by Usman Akinyemi <[email protected]>

Cc [email protected]

Cc Patrick Steinhardt <[email protected]>

Cc [email protected]

Cc Christian Couder <[email protected]>

Cc Eric Sunshine <[email protected]>

Cc Taylor Blau <[email protected]>

We do not tend to use Cc: footers on this list. Also note that as there is a blank line between the Signed-off-by: line and this paragraph the
Signed-off-by: will be ignored by git-interpret-trailers.

Quoted 11 lines

---
  daemon.c    | 14 +++++++++-----
  imap-send.c | 13 ++++++++-----
  merge-ll.c  |  6 ++----
  3 files changed, 19 insertions(+), 14 deletions(-)

daemon.c +0 -0

index cb946e3c95f..3fdb6e83c40 100644

daemon.c int cmd_main(int argc, const char **argv) · line 1311

Quoted 5 lines

daemon.c +1 -1

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

daemon.c int cmd_main(int argc, const char **argv) · line 1312

Quoted 1 line

daemon.c +1 -0

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

daemon.c int cmd_main(int argc, const char **argv) · line 1318

Quoted 7 lines

daemon.c +3 -1

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

daemon.c int cmd_main(int argc, const char **argv) · line 1323

Quoted 8 lines

daemon.c +2 -3

+                     }
                      continue;
              }
              if (skip_prefix(arg, "--max-connections=", &v)) {
-                     max_connections = atoi(v);
-                     if (max_connections < 0)
-                             max_connections = 0;            /* unlimited */
+                     if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) {

This is a faithful translation but if the aim of this series is to detect errors then I think we want to do something like

if (strtol_i(v, 10, &max_connections))
die(...)
if (max_connections < 0)
max_connections = 0; /* unlimited */

Quoted 10 lines

daemon.c +2 -0

+                             max_connections = 0;  /* unlimited */
+                     }
                      continue;
              }
              if (!strcmp(arg, "--strict-paths")) {

imap-send.c +0 -0

index ec68a066877..33b74dfded7 100644

imap-send.c static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, · line 671

Quoted 5 lines

imap-send.c +1 -1

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

imap-send.c static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, · line 676

Quoted 6 lines

imap-send.c +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

imap-send.c static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, · line 690

Quoted 11 lines

imap-send.c +2 -2

                      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

imap-send.c static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd) · line 776

Quoted 9 lines

imap-send.c +1 -1

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

imap-send.c static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd) · line 778

Quoted 2 lines

imap-send.c +2 -0

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

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

Quoted 9 lines

imap-send.c +1 -0

+                     }
                      for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
                              if (cmdp->tag == tag)
                                      goto gottag;

merge-ll.c +0 -0

index 8e63071922b..2bfee0f2c6b 100644

merge-ll.c enum ll_merge_result ll_merge(mmbuffer_t *result_buf, · line 430

Quoted 6 lines

merge-ll.c +1 -2

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

merge-ll.c int ll_merge_marker_size(struct index_state *istate, const char *path) · line 456

Quoted 10 lines

merge-ll.c +1 -2

                      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

Quoted 7 lines · Usman Akinyemi via GitGitGadget

t/t5570-git-daemon.sh +0 -0

index c5f08b67996..c73c2196981 100755
@@ -1,6 +1,6 @@
 #!/bin/sh
 

t/t5570-git-daemon.sh line 6

Quoted 5 lines

t/t5570-git-daemon.sh +1 -1

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

Quoted 4 lines

t/t5570-git-daemon.sh +0 -0

@@ -8,6 +8,31 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 

t/t5570-git-daemon.sh TEST_PASSES_SANITIZE_LEAK=true · line 16

Quoted 7 lines

t/t5570-git-daemon.sh +6 -0

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

Quoted 3 lines

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.

Quoted 12 lines

t/t5570-git-daemon.sh TEST_PASSES_SANITIZE_LEAK=true · line 16

Quoted 10 lines

t/t5570-git-daemon.sh +6 -0

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

Quoted 4 lines · Usman Akinyemi

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 14 lines

Quoted 13 lines

t/t5570-git-daemon.sh TEST_PASSES_SANITIZE_LEAK=true · line 16

Quoted 11 lines

t/t5570-git-daemon.sh +6 -0

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

Quoted 1 line

Will make a change now.

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

Thanks,
Taylor

From: Usman Akinyemi <[email protected]>

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.

merge-ll.c +2 -1

index 8e63071922b..52870226816 100644
@@ -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;
 	}

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 16 lines

merge-ll.c +2 -1

index 8e63071922b..52870226816 100644
@@ -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;
      }

merge-ll.c int ll_merge_marker_size(struct index_state *istate, const char *path) · line 462

Quoted 9 lines

merge-ll.c +2 -1

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

Quoted 9 lines · Patrick Steinhardt

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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 ?

Quoted 10 lines · Usman Akinyemi

Quoted 9 lines · Patrick Steinhardt

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

Quoted 7 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 4 lines

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

Quoted 10 lines · Usman Akinyemi

Quoted 9 lines · Patrick Steinhardt

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 7 lines

Quoted 7 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 4 lines

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

Hi Taylor, when I try to use this warning(_, I was getting some error In the editor

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

Quoted 1 line · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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

Hi Patrick and Usman

Quoted 10 lines · Patrick Steinhardt

Quoted 1 line · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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

Quoted 2 lines

Patrick

Quoted 19 lines · Phillip Wood

Quoted 10 lines · Patrick Steinhardt

Quoted 1 line · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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

Quoted 6 lines

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.

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

Quoted 9 lines

Best Wishes

Phillip

Quoted 2 lines

Patrick

Hi Usman

Quoted 20 lines · Usman Akinyemi

Quoted 19 lines · Phillip Wood

Quoted 10 lines · Patrick Steinhardt

Quoted 1 line · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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

Quoted 8 lines

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 12 lines

Thank you
Usman Akinyemi

Quoted 9 lines

Best Wishes

Phillip

Quoted 2 lines

Patrick

Quoted 40 lines · Phillip Wood

Hi Usman

Quoted 20 lines · Usman Akinyemi

Quoted 19 lines · Phillip Wood

Quoted 10 lines · Patrick Steinhardt

Quoted 1 line · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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

Quoted 8 lines

Quoted 6 lines

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.

Quoted 3 lines

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 ?

Quoted 9 lines

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

Quoted 11 lines

Quoted 5 lines

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

Quoted 5 lines

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.

Quoted 18 lines

Best Wishes

Phillip

Quoted 12 lines

Thank you
Usman Akinyemi

Quoted 9 lines

Best Wishes

Phillip

Quoted 2 lines

Patrick

Quoted 9 lines · Phillip Wood

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.

Quoted 3 lines

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 ?

Quoted 9 lines

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.

From: Usman Akinyemi <[email protected]>

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.

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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

Quoted 10 lines · Patrick Steinhardt

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 6 lines

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 ?

Quoted 18 lines · Usman Akinyemi

Quoted 10 lines · Patrick Steinhardt

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 6 lines

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.

Quoted 1 line

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

Quoted 20 lines · Patrick Steinhardt

Quoted 18 lines · Usman Akinyemi

Quoted 10 lines · Patrick Steinhardt

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 6 lines

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 21 lines · Usman Akinyemi

Quoted 20 lines · Patrick Steinhardt

Quoted 18 lines · Usman Akinyemi

Quoted 10 lines · Patrick Steinhardt

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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

Quoted 18 lines · Usman Akinyemi

Quoted 10 lines · Patrick Steinhardt

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

Quoted 6 lines

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.

Quoted 1 line

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

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

Quoted 4 lines · 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.

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

Quoted 9 lines

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

Quoted 5 lines

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

Quoted 7 lines · Taylor Blau

Quoted 4 lines · 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.

Quoted 2 lines

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

Quoted 8 lines · Usman Akinyemi

Quoted 7 lines · Taylor Blau

Quoted 4 lines · 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.

Quoted 6 lines

Quoted 2 lines

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

daemon.c int cmd_main(int argc, const char **argv) · line 1312

Quoted 11 lines · Usman Akinyemi via GitGitGadget

daemon.c +2 -1

index cb946e3c95f..09a31d2344d 100644
@@ -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