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

Commit Message

Usman Akinyemi Oct. 12, 2024, 11:09 p.m. UTC

From: Usman Akinyemi <[e-mail]>

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=[e-mail]/

Signed-off-by: Usman Akinyemi <[e-mail]>

Cc: [e-mail]
Cc: Patrick Steinhardt <[e-mail]>
Cc: [e-mail]
Cc: Christian Couder <[e-mail]>
Cc: Eric Sunshine <[e-mail]>
Cc: Taylor Blau <[e-mail]>
---
 daemon.c    | 14 +++++++++-----
 imap-send.c | 13 ++++++++-----
 merge-ll.c  |  6 ++----
 3 files changed, 19 insertions(+), 14 deletions(-)

Comments

Usman Akinyemi Oct. 13, 2024, 9:42 a.m. UTC | #1

On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
<[e-mail]> wrote:
>
> From: Usman Akinyemi <[e-mail]>
>
> 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=[e-mail]/
>
> Signed-off-by: Usman Akinyemi <[e-mail]>
>
> Cc: [e-mail]
> Cc: Patrick Steinhardt <[e-mail]>
> Cc: [e-mail]
> Cc: Christian Couder <[e-mail]>
> Cc: Eric Sunshine <[e-mail]>
> Cc: Taylor Blau <[e-mail]>
> ---
>  daemon.c    | 14 +++++++++-----
>  imap-send.c | 13 ++++++++-----
>  merge-ll.c  |  6 ++----
>  3 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index cb946e3c95f..3fdb6e83c40 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1308,17 +1308,21 @@ int cmd_main(int argc, const char **argv)
>                         continue;
>                 }
>                 if (skip_prefix(arg, "--timeout=", &v)) {
> -                       timeout = atoi(v);
> +                       if (strtoul_ui(v, 10, &timeout) < 0) {
> +                               die("'%s': not a valid integer for --timeout", v);
> +                       }
>                         continue;
>                 }
>                 if (skip_prefix(arg, "--init-timeout=", &v)) {
> -                       init_timeout = atoi(v);
> +                       if (strtoul_ui(v, 10, &init_timeout) < 0) {
> +                               die("'%s': not a valid integer for --init-timeout", v);
> +                       }
>                         continue;
>                 }
>                 if (skip_prefix(arg, "--max-connections=", &v)) {
> -                       max_connections = atoi(v);
> -                       if (max_connections < 0)
> -                               max_connections = 0;            /* unlimited */
> +                       if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) {
> +                               max_connections = 0;  /* unlimited */
> +                       }
>                         continue;
>                 }
>                 if (!strcmp(arg, "--strict-paths")) {
> diff --git a/imap-send.c b/imap-send.c
> index ec68a066877..33b74dfded7 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -668,12 +668,12 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
>                 return RESP_BAD;
>         }
>         if (!strcmp("UIDVALIDITY", arg)) {
> -               if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
> +               if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) != 0) {
>                         fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
>                         return RESP_BAD;
>                 }
>         } else if (!strcmp("UIDNEXT", arg)) {
> -               if (!(arg = next_arg(&s)) || !(imap->uidnext = atoi(arg))) {
> +               if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) != 0) {
>                         fprintf(stderr, "IMAP error: malformed NEXTUID status\n");
>                         return RESP_BAD;
>                 }
> @@ -686,8 +686,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
>                 for (; isspace((unsigned char)*p); p++);
>                 fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
>         } else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
> -               if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
> -                   !(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
> +               if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) != 0) ||
> +                       !(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) != 0)) {
>                         fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
>                         return RESP_BAD;
>                 }
> @@ -773,7 +773,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
>                         if (!tcmd)
>                                 return DRV_OK;
>                 } else {
> -                       tag = atoi(arg);
> +                       if (strtol_i(arg, 10, &tag) != 0) {
> +                               fprintf(stderr, "IMAP error: malformed tag %s\n", arg);
> +                               return RESP_BAD;
> +                       }
>                         for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
>                                 if (cmdp->tag == tag)
>                                         goto gottag;
> diff --git a/merge-ll.c b/merge-ll.c
> index 8e63071922b..2bfee0f2c6b 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -427,8 +427,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
>         git_check_attr(istate, path, check);
>         ll_driver_name = check->items[0].value;
>         if (check->items[1].value) {
> -               marker_size = atoi(check->items[1].value);
> -               if (marker_size <= 0)
> +               if (strtol_i(check->items[1].value, 10, &marker_size) != 0 || marker_size <= 0)
>                         marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>         }
>         driver = find_ll_merge_driver(ll_driver_name);
> @@ -454,8 +453,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
>                 check = attr_check_initl("conflict-marker-size", NULL);
>         git_check_attr(istate, path, check);
>         if (check->items[0].value) {
> -               marker_size = atoi(check->items[0].value);
> -               if (marker_size <= 0)
> +               if (strtol_i(check->items[0].value, 10, &marker_size) != 0 || marker_size <= 0)
>                         marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>         }
>         return marker_size;
> --
> gitgitgadget

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

Phillip Wood Oct. 14, 2024, 9 a.m. UTC | #2

Hi Usman

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

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

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

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

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

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

Best Wishes

Phillip

Phillip Wood Oct. 14, 2024, 9:49 a.m. UTC | #3

Hi Usman

On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <[e-mail]>
> 
> Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> and strtol_i() for signed integers across multiple files. This change
> improves error handling and prevents potential integer overflow issues.

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

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

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

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

Great

> This is a #leftoverbit discussed here:
>   https://public-inbox.org/git/CAC4O8c-nuOTS=[e-mail]/
> 
> Signed-off-by: Usman Akinyemi <[e-mail]>
> 
> Cc: [e-mail]
> Cc: Patrick Steinhardt <[e-mail]>
> Cc: [e-mail]
> Cc: Christian Couder <[e-mail]>
> Cc: Eric Sunshine <[e-mail]>
> Cc: Taylor Blau <[e-mail]>

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

> ---
>   daemon.c    | 14 +++++++++-----
>   imap-send.c | 13 ++++++++-----
>   merge-ll.c  |  6 ++----
>   3 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/daemon.c b/daemon.c
> index cb946e3c95f..3fdb6e83c40 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1308,17 +1308,21 @@ int cmd_main(int argc, const char **argv)
>   			continue;
>   		}
>   		if (skip_prefix(arg, "--timeout=", &v)) {
> -			timeout = atoi(v);
> +			if (strtoul_ui(v, 10, &timeout) < 0) {

For functions that return 0 or -1 to indicate success or error 
respectively we use "if (func(args))" to check for errors.

> +				die("'%s': not a valid integer for --timeout", v);

"-1" is a valid integer but it is not a valid timeout, maybe we could 
say something like "invalid timeout '%s', expecting a non-negative integer".

> +			}
>   			continue;
>   		}
>   		if (skip_prefix(arg, "--init-timeout=", &v)) {
> -			init_timeout = atoi(v);
> +			if (strtoul_ui(v, 10, &init_timeout) < 0) {
> +				die("'%s': not a valid integer for --init-timeout", v);

The comments for --timeout apply here as well

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

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

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

> +				max_connections = 0;  /* unlimited */
> +			}
>   			continue;
>   		}
>   		if (!strcmp(arg, "--strict-paths")) {
> diff --git a/imap-send.c b/imap-send.c
> index ec68a066877..33b74dfded7 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -668,12 +668,12 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
>   		return RESP_BAD;
>   	}
>   	if (!strcmp("UIDVALIDITY", arg)) {
> -		if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
> +		if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) != 0) {

The original is checking for a zero return from atoi() which indicates 
an error or that the parsed value was zero. To do that with strtol_i() 
we need to do

	|| (strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity)

The IMAP RFC[1] specifies that UIDVALIDITY should be a non-zero, 
non-negative 32bit integer but I'm not sure we want to start change it's 
type and using strtoul_ui here.

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

>   			fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
>   			return RESP_BAD;
>   		}
>   	} else if (!strcmp("UIDNEXT", arg)) {
> -		if (!(arg = next_arg(&s)) || !(imap->uidnext = atoi(arg))) {
> +		if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) != 0) {

The comments above apply here

>   			fprintf(stderr, "IMAP error: malformed NEXTUID status\n");
>   			return RESP_BAD;
>   		}
> @@ -686,8 +686,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
>   		for (; isspace((unsigned char)*p); p++);
>   		fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
>   	} else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
> -		if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
> -		    !(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
> +		if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) != 0) ||
> +			!(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) != 0)) {

And here

>   			fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
>   			return RESP_BAD;
>   		}
> @@ -773,7 +773,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
>   			if (!tcmd)
>   				return DRV_OK;
>   		} else {
> -			tag = atoi(arg);
> +			if (strtol_i(arg, 10, &tag) != 0) {

To check for an error just use (strtol_i(arg, 10, &tag))

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

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

> +			}
>   			for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
>   				if (cmdp->tag == tag)
>   					goto gottag;
> diff --git a/merge-ll.c b/merge-ll.c
> index 8e63071922b..2bfee0f2c6b 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -427,8 +427,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
>   	git_check_attr(istate, path, check);
>   	ll_driver_name = check->items[0].value;
>   	if (check->items[1].value) {
> -		marker_size = atoi(check->items[1].value);
> -		if (marker_size <= 0)
> +		if (strtol_i(check->items[1].value, 10, &marker_size) != 0 || marker_size <= 0)

Here I think we want to return an error if we cannot parse the marker 
size and then set the default if the marker size is <= 0 like we do for 
the max_connections code in daemon.c above.

>   			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>   	}
>   	driver = find_ll_merge_driver(ll_driver_name);
> @@ -454,8 +453,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
>   		check = attr_check_initl("conflict-marker-size", NULL);
>   	git_check_attr(istate, path, check);
>   	if (check->items[0].value) {
> -		marker_size = atoi(check->items[0].value);
> -		if (marker_size <= 0)
> +		if (strtol_i(check->items[0].value, 10, &marker_size) != 0 || marker_size <= 0)

And the same here

Thanks for working on this, it will be a useful improvement to our 
integer parsing. I think you've got the basic idea, it just needs a bit 
of polish

Phillip

Kristoffer Haugsbakk Oct. 14, 2024, 10:06 a.m. UTC | #4

>> Cc: [e-mail]
>> Cc: Patrick Steinhardt <[e-mail]>
>> Cc: [e-mail]
>> Cc: Christian Couder <[e-mail]>
>> Cc: Eric Sunshine <[e-mail]>
>> Cc: Taylor Blau <[e-mail]>
>
> 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.

Patrick Steinhardt Oct. 14, 2024, 10:53 a.m. UTC | #5

On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> <[e-mail]> wrote:
> >
> > From: Usman Akinyemi <[e-mail]>
> >
> > Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> > and strtol_i() for signed integers across multiple files. This change
> > improves error handling and prevents potential integer overflow issues.
> >
> > The following files were updated:
> > - daemon.c: Update parsing of --timeout, --init-timeout, and
> >   --max-connections
> > - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
> >   tags
> > - merge-ll.c: Enhance parsing of marker size in ll_merge and
> >   ll_merge_marker_size

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

> > This change allows for better error detection when parsing integer
> > values from command-line arguments and IMAP responses, making the code
> > more robust and secure.
> >
> > This is a #leftoverbit discussed here:
> >  https://public-inbox.org/git/CAC4O8c-nuOTS=[e-mail]/
> >
> > Signed-off-by: Usman Akinyemi <[e-mail]>
> >
> > Cc: [e-mail]
> > Cc: Patrick Steinhardt <[e-mail]>
> > Cc: [e-mail]
> > Cc: Christian Couder <[e-mail]>
> > Cc: Eric Sunshine <[e-mail]>
> > Cc: Taylor Blau <[e-mail]>

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

> > ---
> >  daemon.c    | 14 +++++++++-----
> >  imap-send.c | 13 ++++++++-----
> >  merge-ll.c  |  6 ++----
> >  3 files changed, 19 insertions(+), 14 deletions(-)
> >
> > diff --git a/daemon.c b/daemon.c
> > index cb946e3c95f..3fdb6e83c40 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1308,17 +1308,21 @@ int cmd_main(int argc, const char **argv)
> >                         continue;
> >                 }
> >                 if (skip_prefix(arg, "--timeout=", &v)) {
> > -                       timeout = atoi(v);
> > +                       if (strtoul_ui(v, 10, &timeout) < 0) {
> > +                               die("'%s': not a valid integer for --timeout", v);
> > +                       }
> >                         continue;
> >                 }

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

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

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

Patrick

Phillip Wood Oct. 14, 2024, 1:48 p.m. UTC | #6

Hi Kristoffer

On 14/10/2024 11:06, Kristoffer Haugsbakk wrote:
>>> Cc: [e-mail]
>>> Cc: Patrick Steinhardt <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: Christian Couder <[e-mail]>
>>> Cc: Eric Sunshine <[e-mail]>
>>> Cc: Taylor Blau <[e-mail]>
>>
>> We do not tend to use Cc: footers on this list. Also note that as there
>> is a blank line between the Signed-off-by: line and this paragraph the
>> Signed-off-by: will be ignored by git-interpret-trailers.
> 
> I thought that gitgitgadget checked for missing sign-off.  I’ve seen
> that message before at least.

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

Best Wishes

Phillip

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

Phillip Wood Oct. 14, 2024, 1:57 p.m. UTC | #7

On 14/10/2024 11:53, Patrick Steinhardt wrote:
> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
>> <[e-mail]> wrote:
>>>
>>> From: Usman Akinyemi <[e-mail]>
>>>
>>> Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
>>> and strtol_i() for signed integers across multiple files. This change
>>> improves error handling and prevents potential integer overflow issues.
>>>
>>> The following files were updated:
>>> - daemon.c: Update parsing of --timeout, --init-timeout, and
>>>    --max-connections
>>> - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
>>>    tags
>>> - merge-ll.c: Enhance parsing of marker size in ll_merge and
>>>    ll_merge_marker_size
> 
> To me it's always an indicator that something should be split up across
> multiple commits once you have a bulleted list of changes in your commit
> message.

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

Best Wishes

Phillip

>>> This change allows for better error detection when parsing integer
>>> values from command-line arguments and IMAP responses, making the code
>>> more robust and secure.
>>>
>>> This is a #leftoverbit discussed here:
>>>   https://public-inbox.org/git/CAC4O8c-nuOTS=[e-mail]/
>>>
>>> Signed-off-by: Usman Akinyemi <[e-mail]>
>>>
>>> Cc: [e-mail]
>>> Cc: Patrick Steinhardt <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: Christian Couder <[e-mail]>
>>> Cc: Eric Sunshine <[e-mail]>
>>> Cc: Taylor Blau <[e-mail]>
> 
> The Cc annotations shouldn't be part of the commit message. If you want
> to Cc specific folks you should rather do it e.g. on the command line or
> whatever you use to send out the patches. Otherwise, these will all end
> up in our history.
> 
>>> ---
>>>   daemon.c    | 14 +++++++++-----
>>>   imap-send.c | 13 ++++++++-----
>>>   merge-ll.c  |  6 ++----
>>>   3 files changed, 19 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/daemon.c b/daemon.c
>>> index cb946e3c95f..3fdb6e83c40 100644
>>> --- a/daemon.c
>>> +++ b/daemon.c
>>> @@ -1308,17 +1308,21 @@ int cmd_main(int argc, const char **argv)
>>>                          continue;
>>>                  }
>>>                  if (skip_prefix(arg, "--timeout=", &v)) {
>>> -                       timeout = atoi(v);
>>> +                       if (strtoul_ui(v, 10, &timeout) < 0) {
>>> +                               die("'%s': not a valid integer for --timeout", v);
>>> +                       }
>>>                          continue;
>>>                  }
> 
> We don't use braces around single-line statements. It would also help to
> explain whether this is fixing a bug and, if it does, then it would be
> nice to have a testcase that demonstrates the behaviour. The same is
> true for the other sites that you convert.
> 
> [snip]
>> I also want to ask if this is the right way to send another patch as I
>> noticed that it is showing my previous patch which is not related to
>> this. Thank you.
> 
> You shouldn't ever include patches from another patch series. I guess
> tha problem here is that you created all of your work on the same
> branch. I'd recommend to use separate feature branches for every series
> you are working on. In general, these branches should start from the
> current "main" branch.
> 
> Patrick
>

Patrick Steinhardt Oct. 14, 2024, 2 p.m. UTC | #8

On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:
> On 14/10/2024 11:53, Patrick Steinhardt wrote:
> > On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> > > On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> > > <[e-mail]> wrote:
> > > > 
> > > > From: Usman Akinyemi <[e-mail]>
> > > > 
> > > > Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> > > > and strtol_i() for signed integers across multiple files. This change
> > > > improves error handling and prevents potential integer overflow issues.
> > > > 
> > > > The following files were updated:
> > > > - daemon.c: Update parsing of --timeout, --init-timeout, and
> > > >    --max-connections
> > > > - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
> > > >    tags
> > > > - merge-ll.c: Enhance parsing of marker size in ll_merge and
> > > >    ll_merge_marker_size
> > 
> > To me it's always an indicator that something should be split up across
> > multiple commits once you have a bulleted list of changes in your commit
> > message.
> 
> Agreed, but I think in this case there is a common theme (converting atoi()
> to a safer alternative) and the problem is with the commit message listing
> which files have changed rather than unrelated code changes being grouped
> together. This patch could be split up and if there were many more atoi()
> conversions it would need to be split to prevent it being too long but I
> don't think its essential to do so.

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

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

Patrick

Phillip Wood Oct. 14, 2024, 2:55 p.m. UTC | #9

On 14/10/2024 15:00, Patrick Steinhardt wrote:
> On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:
>> On 14/10/2024 11:53, Patrick Steinhardt wrote:
>>> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
>>>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
>>>> <[e-mail]> wrote:
>>>>>
>>>>> From: Usman Akinyemi <[e-mail]>
>>>>>
>>>>> Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
>>>>> and strtol_i() for signed integers across multiple files. This change
>>>>> improves error handling and prevents potential integer overflow issues.
>>>>>
>>>>> The following files were updated:
>>>>> - daemon.c: Update parsing of --timeout, --init-timeout, and
>>>>>     --max-connections
>>>>> - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
>>>>>     tags
>>>>> - merge-ll.c: Enhance parsing of marker size in ll_merge and
>>>>>     ll_merge_marker_size
>>>
>>> To me it's always an indicator that something should be split up across
>>> multiple commits once you have a bulleted list of changes in your commit
>>> message.
>>
>> Agreed, but I think in this case there is a common theme (converting atoi()
>> to a safer alternative) and the problem is with the commit message listing
>> which files have changed rather than unrelated code changes being grouped
>> together. This patch could be split up and if there were many more atoi()
>> conversions it would need to be split to prevent it being too long but I
>> don't think its essential to do so.
> 
> In theory I agree. In practice I think we should have better
> explanations why the respective conversions are fine and whether this is
> fixing a bug or not. And if it is fixing bugs I'd also like to see tests
> added to the tree.

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

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

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

Phillip

> Patrick
>

Usman Akinyemi Oct. 14, 2024, 3:56 p.m. UTC | #10

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

Usman Akinyemi Oct. 14, 2024, 4:03 p.m. UTC | #11

On Mon, Oct 14, 2024 at 10:53 AM Patrick Steinhardt <[e-mail]> wrote:
>
> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> > On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> > <[e-mail]> wrote:
> > >
> > > From: Usman Akinyemi <[e-mail]>
> > >
> > > Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> > > and strtol_i() for signed integers across multiple files. This change
> > > improves error handling and prevents potential integer overflow issues.
> > >
> > > The following files were updated:
> > > - daemon.c: Update parsing of --timeout, --init-timeout, and
> > >   --max-connections
> > > - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
> > >   tags
> > > - merge-ll.c: Enhance parsing of marker size in ll_merge and
> > >   ll_merge_marker_size
>
> To me it's always an indicator that something should be split up across
> multiple commits once you have a bulleted list of changes in your commit
> message.
>
> > > This change allows for better error detection when parsing integer
> > > values from command-line arguments and IMAP responses, making the code
> > > more robust and secure.
> > >
> > > This is a #leftoverbit discussed here:
> > >  https://public-inbox.org/git/CAC4O8c-nuOTS=[e-mail]/
> > >
> > > Signed-off-by: Usman Akinyemi <[e-mail]>
> > >
> > > Cc: [e-mail]
> > > Cc: Patrick Steinhardt <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: Christian Couder <[e-mail]>
> > > Cc: Eric Sunshine <[e-mail]>
> > > Cc: Taylor Blau <[e-mail]>
>
> The Cc annotations shouldn't be part of the commit message. If you want
> to Cc specific folks you should rather do it e.g. on the command line or
> whatever you use to send out the patches. Otherwise, these will all end
> up in our history.
Thanks for this, I thought the gitgitgadget automatically use the Cc
from the commit message.
>
> > > ---
> > >  daemon.c    | 14 +++++++++-----
> > >  imap-send.c | 13 ++++++++-----
> > >  merge-ll.c  |  6 ++----
> > >  3 files changed, 19 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/daemon.c b/daemon.c
> > > index cb946e3c95f..3fdb6e83c40 100644
> > > --- a/daemon.c
> > > +++ b/daemon.c
> > > @@ -1308,17 +1308,21 @@ int cmd_main(int argc, const char **argv)
> > >                         continue;
> > >                 }
> > >                 if (skip_prefix(arg, "--timeout=", &v)) {
> > > -                       timeout = atoi(v);
> > > +                       if (strtoul_ui(v, 10, &timeout) < 0) {
> > > +                               die("'%s': not a valid integer for --timeout", v);
> > > +                       }
> > >                         continue;
> > >                 }
>
> We don't use braces around single-line statements. It would also help to
> explain whether this is fixing a bug and, if it does, then it would be
> nice to have a testcase that demonstrates the behaviour. The same is
> true for the other sites that you convert.
>
I was going to add testcase, I sent this patch to ensure I am going in
the right direction.
> [snip]
> > I also want to ask if this is the right way to send another patch as I
> > noticed that it is showing my previous patch which is not related to
> > this. Thank you.
>
> You shouldn't ever include patches from another patch series. I guess
> tha problem here is that you created all of your work on the same
> branch. I'd recommend to use separate feature branches for every series
> you are working on. In general, these branches should start from the
> current "main" branch.
>
> Patrick
Thanks Patrick. I actually created a new branch for this branch, my
mistake was not basing it on the master branch. I was a little bit
confused. But, now I understand better. Thanks.

Usman Akinyemi Oct. 14, 2024, 4:13 p.m. UTC | #12

On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <[e-mail]> wrote:
>
> On 14/10/2024 15:00, Patrick Steinhardt wrote:
> > On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:
> >> On 14/10/2024 11:53, Patrick Steinhardt wrote:
> >>> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> >>>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> >>>> <[e-mail]> wrote:
> >>>>>
> >>>>> From: Usman Akinyemi <[e-mail]>
> >>>>>
> >>>>> Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> >>>>> and strtol_i() for signed integers across multiple files. This change
> >>>>> improves error handling and prevents potential integer overflow issues.
> >>>>>
> >>>>> The following files were updated:
> >>>>> - daemon.c: Update parsing of --timeout, --init-timeout, and
> >>>>>     --max-connections
> >>>>> - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
> >>>>>     tags
> >>>>> - merge-ll.c: Enhance parsing of marker size in ll_merge and
> >>>>>     ll_merge_marker_size
> >>>
> >>> To me it's always an indicator that something should be split up across
> >>> multiple commits once you have a bulleted list of changes in your commit
> >>> message.
> >>
> >> Agreed, but I think in this case there is a common theme (converting atoi()
> >> to a safer alternative) and the problem is with the commit message listing
> >> which files have changed rather than unrelated code changes being grouped
> >> together. This patch could be split up and if there were many more atoi()
> >> conversions it would need to be split to prevent it being too long but I
> >> don't think its essential to do so.
> >
> > In theory I agree. In practice I think we should have better
> > explanations why the respective conversions are fine and whether this is
> > fixing a bug or not. And if it is fixing bugs I'd also like to see tests
> > added to the tree.
>
> I'm not sure if I would describe any of the changes as fixing bugs. The
> option and config parsing code becomes stricter so I guess you could say
> it was a bug to accept any old rubbish and treat it as zero before. The
> imap code that's changed all rejected zero anyway apart from the tag
> parsing so maybe accepting the changes to the tag parsing are fixing a bug.
>
> > And by the time we got there it makes sense to split up commits.
>
> Yes if we start adding tests then it is worth splitting them up, I'm not
> sure we have anyway of testing the imap changes but it would be worth
> testing the other changes though.
>
> Phillip
>
> > Patrick
> >
>
I got this from a leftoverbit which the main issue was reported as
bug. https://public-inbox.org/git/CAC4O8c-nuOTS=[e-mail]/

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

Usman Akinyemi Oct. 14, 2024, 4:26 p.m. UTC | #13

On Mon, Oct 14, 2024 at 4:13 PM Usman Akinyemi
<[e-mail]> wrote:
>
> On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <[e-mail]> wrote:
> >
> > On 14/10/2024 15:00, Patrick Steinhardt wrote:
> > > On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:
> > >> On 14/10/2024 11:53, Patrick Steinhardt wrote:
> > >>> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> > >>>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> > >>>> <[e-mail]> wrote:
> > >>>>>
> > >>>>> From: Usman Akinyemi <[e-mail]>
> > >>>>>
> > >>>>> Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> > >>>>> and strtol_i() for signed integers across multiple files. This change
> > >>>>> improves error handling and prevents potential integer overflow issues.
> > >>>>>
> > >>>>> The following files were updated:
> > >>>>> - daemon.c: Update parsing of --timeout, --init-timeout, and
> > >>>>>     --max-connections
> > >>>>> - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
> > >>>>>     tags
> > >>>>> - merge-ll.c: Enhance parsing of marker size in ll_merge and
> > >>>>>     ll_merge_marker_size
> > >>>
> > >>> To me it's always an indicator that something should be split up across
> > >>> multiple commits once you have a bulleted list of changes in your commit
> > >>> message.
> > >>
> > >> Agreed, but I think in this case there is a common theme (converting atoi()
> > >> to a safer alternative) and the problem is with the commit message listing
> > >> which files have changed rather than unrelated code changes being grouped
> > >> together. This patch could be split up and if there were many more atoi()
> > >> conversions it would need to be split to prevent it being too long but I
> > >> don't think its essential to do so.
> > >
> > > In theory I agree. In practice I think we should have better
> > > explanations why the respective conversions are fine and whether this is
> > > fixing a bug or not. And if it is fixing bugs I'd also like to see tests
> > > added to the tree.
> >
> > I'm not sure if I would describe any of the changes as fixing bugs. The
> > option and config parsing code becomes stricter so I guess you could say
> > it was a bug to accept any old rubbish and treat it as zero before. The
> > imap code that's changed all rejected zero anyway apart from the tag
> > parsing so maybe accepting the changes to the tag parsing are fixing a bug.
> >
> > > And by the time we got there it makes sense to split up commits.
> >
> > Yes if we start adding tests then it is worth splitting them up, I'm not
> > sure we have anyway of testing the imap changes but it would be worth
> > testing the other changes though.
> >
> > Phillip
> >
> > > Patrick
> > >
> >
> I got this from a leftoverbit which the main issue was reported as
> bug. https://public-inbox.org/git/CAC4O8c-nuOTS=[e-mail]/
>
> For the test, I should have the test as another patch right ?
> Thanks.
Also, do I need to add the reference which mentions the leftoverbit in
the commit message?

Usman Akinyemi Oct. 14, 2024, 6:20 p.m. UTC | #14

On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood <[e-mail]> wrote:
>
> Hi Usman
>
> On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <[e-mail]>
> >
> > Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> > and strtol_i() for signed integers across multiple files. This change
> > improves error handling and prevents potential integer overflow issues.
>
> This paragraph is good as it explains why you are making this change
>
> > The following files were updated:
> > - daemon.c: Update parsing of --timeout, --init-timeout, and
> >    --max-connections
> > - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
> >    tags
> > - merge-ll.c: Enhance parsing of marker size in ll_merge and
> >    ll_merge_marker_size
>
> This information is not really needed in the commit message as it is
> shown in the diff.
>
> > This change allows for better error detection when parsing integer
> > values from command-line arguments and IMAP responses, making the code
> > more robust and secure.
>
> Great
>
> > This is a #leftoverbit discussed here:
> >   https://public-inbox.org/git/CAC4O8c-nuOTS=[e-mail]/
> >
> > Signed-off-by: Usman Akinyemi <[e-mail]>
> >
> > Cc: [e-mail]
> > Cc: Patrick Steinhardt <[e-mail]>
> > Cc: [e-mail]
> > Cc: Christian Couder <[e-mail]>
> > Cc: Eric Sunshine <[e-mail]>
> > Cc: Taylor Blau <[e-mail]>
>
> We do not tend to use Cc: footers on this list. Also note that as there
> is a blank line between the Signed-off-by: line and this paragraph the
> Signed-off-by: will be ignored by git-interpret-trailers.
>
> > ---
> >   daemon.c    | 14 +++++++++-----
> >   imap-send.c | 13 ++++++++-----
> >   merge-ll.c  |  6 ++----
> >   3 files changed, 19 insertions(+), 14 deletions(-)
> >
> > diff --git a/daemon.c b/daemon.c
> > index cb946e3c95f..3fdb6e83c40 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1308,17 +1308,21 @@ int cmd_main(int argc, const char **argv)
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--timeout=", &v)) {
> > -                     timeout = atoi(v);
> > +                     if (strtoul_ui(v, 10, &timeout) < 0) {
>
> For functions that return 0 or -1 to indicate success or error
> respectively we use "if (func(args))" to check for errors.
>
> > +                             die("'%s': not a valid integer for --timeout", v);
>
> "-1" is a valid integer but it is not a valid timeout, maybe we could
> say something like "invalid timeout '%s', expecting a non-negative integer".
>
> > +                     }
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--init-timeout=", &v)) {
> > -                     init_timeout = atoi(v);
> > +                     if (strtoul_ui(v, 10, &init_timeout) < 0) {
> > +                             die("'%s': not a valid integer for --init-timeout", v);
>
> The comments for --timeout apply here as well
>
> > +                     }
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--max-connections=", &v)) {
> > -                     max_connections = atoi(v);
> > -                     if (max_connections < 0)
> > -                             max_connections = 0;            /* unlimited */
> > +                     if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) {
>
> This is a faithful translation but if the aim of this series is to
> detect errors then I think we want to do something like
>
>         if (strtol_i(v, 10, &max_connections))
>                 die(...)
ohh, what I understand in this part of the code is intended to set
max_connections to 0 if the value it is currently set to is invalid,
such as containing letters or being negative. Your suggestion implies
that we should return an error to indicate that letters are not
accepted.
>         if (max_connections < 0)
>                 max_connections = 0; /* unlimited */
>
> > +                             max_connections = 0;  /* unlimited */
> > +                     }
> >                       continue;
> >               }
> >               if (!strcmp(arg, "--strict-paths")) {
> > diff --git a/imap-send.c b/imap-send.c
> > index ec68a066877..33b74dfded7 100644
> > --- a/imap-send.c
> > +++ b/imap-send.c
> > @@ -668,12 +668,12 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
> >               return RESP_BAD;
> >       }
> >       if (!strcmp("UIDVALIDITY", arg)) {
> > -             if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
> > +             if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) != 0) {
>
> The original is checking for a zero return from atoi() which indicates
> an error or that the parsed value was zero. To do that with strtol_i()
> we need to do
>
>         || (strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity)
>
> The IMAP RFC[1] specifies that UIDVALIDITY should be a non-zero,
> non-negative 32bit integer but I'm not sure we want to start change it's
> type and using strtoul_ui here.
>
> [1] https://www.rfc-editor.org/rfc/rfc3501#section-2.3.1.1
>
> >                       fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
> >                       return RESP_BAD;
> >               }
> >       } else if (!strcmp("UIDNEXT", arg)) {
> > -             if (!(arg = next_arg(&s)) || !(imap->uidnext = atoi(arg))) {
> > +             if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) != 0) {
>
> The comments above apply here
>
> >                       fprintf(stderr, "IMAP error: malformed NEXTUID status\n");
> >                       return RESP_BAD;
> >               }
> > @@ -686,8 +686,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
> >               for (; isspace((unsigned char)*p); p++);
> >               fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
> >       } else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
> > -             if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
> > -                 !(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
> > +             if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) != 0) ||
> > +                     !(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) != 0)) {
>
> And here
>
> >                       fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
> >                       return RESP_BAD;
> >               }
> > @@ -773,7 +773,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
> >                       if (!tcmd)
> >                               return DRV_OK;
> >               } else {
> > -                     tag = atoi(arg);
> > +                     if (strtol_i(arg, 10, &tag) != 0) {
>
> To check for an error just use (strtol_i(arg, 10, &tag))
>
> > +                             fprintf(stderr, "IMAP error: malformed tag %s\n", arg);
> > +                             return RESP_BAD;
>
> This matches the error below so I assume it's good.
>
> > +                     }
> >                       for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
> >                               if (cmdp->tag == tag)
> >                                       goto gottag;
> > diff --git a/merge-ll.c b/merge-ll.c
> > index 8e63071922b..2bfee0f2c6b 100644
> > --- a/merge-ll.c
> > +++ b/merge-ll.c
> > @@ -427,8 +427,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
> >       git_check_attr(istate, path, check);
> >       ll_driver_name = check->items[0].value;
> >       if (check->items[1].value) {
> > -             marker_size = atoi(check->items[1].value);
> > -             if (marker_size <= 0)
> > +             if (strtol_i(check->items[1].value, 10, &marker_size) != 0 || marker_size <= 0)
>
> Here I think we want to return an error if we cannot parse the marker
> size and then set the default if the marker size is <= 0 like we do for
> the max_connections code in daemon.c above.
>
> >                       marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> >       }
> >       driver = find_ll_merge_driver(ll_driver_name);
> > @@ -454,8 +453,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
> >               check = attr_check_initl("conflict-marker-size", NULL);
> >       git_check_attr(istate, path, check);
> >       if (check->items[0].value) {
> > -             marker_size = atoi(check->items[0].value);
> > -             if (marker_size <= 0)
> > +             if (strtol_i(check->items[0].value, 10, &marker_size) != 0 || marker_size <= 0)
>
> And the same here
>
> Thanks for working on this, it will be a useful improvement to our
> integer parsing. I think you've got the basic idea, it just needs a bit
> of polish
>
> Phillip
>

Phillip Wood Oct. 14, 2024, 6:30 p.m. UTC | #15

On 14/10/2024 19:20, Usman Akinyemi wrote:
> On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood <[e-mail]> wrote:
>> On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:
>>> From: Usman Akinyemi <[e-mail]>
>>>                }
>>>                if (skip_prefix(arg, "--max-connections=", &v)) {
>>> -                     max_connections = atoi(v);
>>> -                     if (max_connections < 0)
>>> -                             max_connections = 0;            /* unlimited */
>>> +                     if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) {
>>
>> This is a faithful translation but if the aim of this series is to
>> detect errors then I think we want to do something like
>>
>>          if (strtol_i(v, 10, &max_connections))
>>                  die(...)
> ohh, what I understand in this part of the code is intended to set
> max_connections to 0 if the value it is currently set to is invalid,
> such as containing letters or being negative. Your suggestion implies
> that we should return an error to indicate that letters are not
> accepted.

Yes - I don't think we should be accepting any old rubbish when we 
expect a number

Best Wishes

Phillip

>>          if (max_connections < 0)
>>                  max_connections = 0; /* unlimited */
>>
>>> +                             max_connections = 0;  /* unlimited */
>>> +                     }
>>>                        continue;
>>>                }
>>>                if (!strcmp(arg, "--strict-paths")) {
>>> diff --git a/imap-send.c b/imap-send.c
>>> index ec68a066877..33b74dfded7 100644
>>> --- a/imap-send.c
>>> +++ b/imap-send.c
>>> @@ -668,12 +668,12 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
>>>                return RESP_BAD;
>>>        }
>>>        if (!strcmp("UIDVALIDITY", arg)) {
>>> -             if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
>>> +             if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) != 0) {
>>
>> The original is checking for a zero return from atoi() which indicates
>> an error or that the parsed value was zero. To do that with strtol_i()
>> we need to do
>>
>>          || (strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity)
>>
>> The IMAP RFC[1] specifies that UIDVALIDITY should be a non-zero,
>> non-negative 32bit integer but I'm not sure we want to start change it's
>> type and using strtoul_ui here.
>>
>> [1] https://www.rfc-editor.org/rfc/rfc3501#section-2.3.1.1
>>
>>>                        fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
>>>                        return RESP_BAD;
>>>                }
>>>        } else if (!strcmp("UIDNEXT", arg)) {
>>> -             if (!(arg = next_arg(&s)) || !(imap->uidnext = atoi(arg))) {
>>> +             if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) != 0) {
>>
>> The comments above apply here
>>
>>>                        fprintf(stderr, "IMAP error: malformed NEXTUID status\n");
>>>                        return RESP_BAD;
>>>                }
>>> @@ -686,8 +686,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
>>>                for (; isspace((unsigned char)*p); p++);
>>>                fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
>>>        } else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
>>> -             if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
>>> -                 !(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
>>> +             if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) != 0) ||
>>> +                     !(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) != 0)) {
>>
>> And here
>>
>>>                        fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
>>>                        return RESP_BAD;
>>>                }
>>> @@ -773,7 +773,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
>>>                        if (!tcmd)
>>>                                return DRV_OK;
>>>                } else {
>>> -                     tag = atoi(arg);
>>> +                     if (strtol_i(arg, 10, &tag) != 0) {
>>
>> To check for an error just use (strtol_i(arg, 10, &tag))
>>
>>> +                             fprintf(stderr, "IMAP error: malformed tag %s\n", arg);
>>> +                             return RESP_BAD;
>>
>> This matches the error below so I assume it's good.
>>
>>> +                     }
>>>                        for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
>>>                                if (cmdp->tag == tag)
>>>                                        goto gottag;
>>> diff --git a/merge-ll.c b/merge-ll.c
>>> index 8e63071922b..2bfee0f2c6b 100644
>>> --- a/merge-ll.c
>>> +++ b/merge-ll.c
>>> @@ -427,8 +427,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
>>>        git_check_attr(istate, path, check);
>>>        ll_driver_name = check->items[0].value;
>>>        if (check->items[1].value) {
>>> -             marker_size = atoi(check->items[1].value);
>>> -             if (marker_size <= 0)
>>> +             if (strtol_i(check->items[1].value, 10, &marker_size) != 0 || marker_size <= 0)
>>
>> Here I think we want to return an error if we cannot parse the marker
>> size and then set the default if the marker size is <= 0 like we do for
>> the max_connections code in daemon.c above.
>>
>>>                        marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>>>        }
>>>        driver = find_ll_merge_driver(ll_driver_name);
>>> @@ -454,8 +453,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
>>>                check = attr_check_initl("conflict-marker-size", NULL);
>>>        git_check_attr(istate, path, check);
>>>        if (check->items[0].value) {
>>> -             marker_size = atoi(check->items[0].value);
>>> -             if (marker_size <= 0)
>>> +             if (strtol_i(check->items[0].value, 10, &marker_size) != 0 || marker_size <= 0)
>>
>> And the same here
>>
>> Thanks for working on this, it will be a useful improvement to our
>> integer parsing. I think you've got the basic idea, it just needs a bit
>> of polish
>>
>> Phillip
>>

Phillip Wood Oct. 14, 2024, 6:36 p.m. UTC | #16

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

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

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

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


Best Wishes

Phillip

Usman Akinyemi Oct. 15, 2024, 3:17 p.m. UTC | #17

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

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

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

Thank you.
Usman Akinyemi


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

Taylor Blau Oct. 15, 2024, 4:19 p.m. UTC | #18

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

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

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

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

Thanks,
Taylor

Phillip Wood Oct. 15, 2024, 6:28 p.m. UTC | #19

Hi Usman

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

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

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

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

Best Wishes

Phillip

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

Phillip Wood Oct. 16, 2024, 9:20 a.m. UTC | #20

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

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

Best Wishes

Phillip

Usman Akinyemi Oct. 16, 2024, 5:58 p.m. UTC | #21

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

Usman Akinyemi Oct. 16, 2024, 6 p.m. UTC | #22

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

Usman Akinyemi Oct. 17, 2024, 11:16 a.m. UTC | #23

On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood <[e-mail]> wrote:
>
> Hi Usman
>
> On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <[e-mail]>
> >
> > Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> > and strtol_i() for signed integers across multiple files. This change
> > improves error handling and prevents potential integer overflow issues.
>
> This paragraph is good as it explains why you are making this change
>
> > The following files were updated:
> > - daemon.c: Update parsing of --timeout, --init-timeout, and
> >    --max-connections
> > - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
> >    tags
> > - merge-ll.c: Enhance parsing of marker size in ll_merge and
> >    ll_merge_marker_size
>
> This information is not really needed in the commit message as it is
> shown in the diff.
>
> > This change allows for better error detection when parsing integer
> > values from command-line arguments and IMAP responses, making the code
> > more robust and secure.
>
> Great
>
> > This is a #leftoverbit discussed here:
> >   https://public-inbox.org/git/CAC4O8c-nuOTS=[e-mail]/
> >
> > Signed-off-by: Usman Akinyemi <[e-mail]>
> >
> > Cc: [e-mail]
> > Cc: Patrick Steinhardt <[e-mail]>
> > Cc: [e-mail]
> > Cc: Christian Couder <[e-mail]>
> > Cc: Eric Sunshine <[e-mail]>
> > Cc: Taylor Blau <[e-mail]>
>
> We do not tend to use Cc: footers on this list. Also note that as there
> is a blank line between the Signed-off-by: line and this paragraph the
> Signed-off-by: will be ignored by git-interpret-trailers.
>
> > ---
> >   daemon.c    | 14 +++++++++-----
> >   imap-send.c | 13 ++++++++-----
> >   merge-ll.c  |  6 ++----
> >   3 files changed, 19 insertions(+), 14 deletions(-)
> >
> > diff --git a/daemon.c b/daemon.c
> > index cb946e3c95f..3fdb6e83c40 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1308,17 +1308,21 @@ int cmd_main(int argc, const char **argv)
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--timeout=", &v)) {
> > -                     timeout = atoi(v);
> > +                     if (strtoul_ui(v, 10, &timeout) < 0) {
>
> For functions that return 0 or -1 to indicate success or error
> respectively we use "if (func(args))" to check for errors.
>
> > +                             die("'%s': not a valid integer for --timeout", v);
>
> "-1" is a valid integer but it is not a valid timeout, maybe we could
> say something like "invalid timeout '%s', expecting a non-negative integer".
>
> > +                     }
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--init-timeout=", &v)) {
> > -                     init_timeout = atoi(v);
> > +                     if (strtoul_ui(v, 10, &init_timeout) < 0) {
> > +                             die("'%s': not a valid integer for --init-timeout", v);
>
> The comments for --timeout apply here as well
>
> > +                     }
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--max-connections=", &v)) {
> > -                     max_connections = atoi(v);
> > -                     if (max_connections < 0)
> > -                             max_connections = 0;            /* unlimited */
> > +                     if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) {
>
> This is a faithful translation but if the aim of this series is to
> detect errors then I think we want to do something like
>
>         if (strtol_i(v, 10, &max_connections))
>                 die(...)
>         if (max_connections < 0)
>                 max_connections = 0; /* unlimited */
>
> > +                             max_connections = 0;  /* unlimited */
> > +                     }
> >                       continue;
> >               }
> >               if (!strcmp(arg, "--strict-paths")) {
> > diff --git a/imap-send.c b/imap-send.c
> > index ec68a066877..33b74dfded7 100644
> > --- a/imap-send.c
> > +++ b/imap-send.c
> > @@ -668,12 +668,12 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
> >               return RESP_BAD;
> >       }
> >       if (!strcmp("UIDVALIDITY", arg)) {
> > -             if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
> > +             if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) != 0) {
>
> The original is checking for a zero return from atoi() which indicates
> an error or that the parsed value was zero. To do that with strtol_i()
> we need to do
>
>         || (strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity)
>
> The IMAP RFC[1] specifies that UIDVALIDITY should be a non-zero,
> non-negative 32bit integer but I'm not sure we want to start change it's
> type and using strtoul_ui here.
Hello, regarding this. I used strtol_i here as ctx->uidvalidity
was declared to be int so, the strtoul_ui complained as it was
expecting an unsigned int.
My suggestion will be to leave it as strol_i and use this comparison
(strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity), what do you think ?
>
> [1] https://www.rfc-editor.org/rfc/rfc3501#section-2.3.1.1
>
> >                       fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
> >                       return RESP_BAD;
> >               }
> >       } else if (!strcmp("UIDNEXT", arg)) {
> > -             if (!(arg = next_arg(&s)) || !(imap->uidnext = atoi(arg))) {
> > +             if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) != 0) {
>
> The comments above apply here
>
> >                       fprintf(stderr, "IMAP error: malformed NEXTUID status\n");
> >                       return RESP_BAD;
> >               }
> > @@ -686,8 +686,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
> >               for (; isspace((unsigned char)*p); p++);
> >               fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
> >       } else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
> > -             if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
> > -                 !(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
> > +             if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) != 0) ||
> > +                     !(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) != 0)) {
>
> And here
>
> >                       fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
> >                       return RESP_BAD;
> >               }
> > @@ -773,7 +773,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
> >                       if (!tcmd)
> >                               return DRV_OK;
> >               } else {
> > -                     tag = atoi(arg);
> > +                     if (strtol_i(arg, 10, &tag) != 0) {
>
> To check for an error just use (strtol_i(arg, 10, &tag))
>
> > +                             fprintf(stderr, "IMAP error: malformed tag %s\n", arg);
> > +                             return RESP_BAD;
>
> This matches the error below so I assume it's good.
>
> > +                     }
> >                       for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
> >                               if (cmdp->tag == tag)
> >                                       goto gottag;
> > diff --git a/merge-ll.c b/merge-ll.c
> > index 8e63071922b..2bfee0f2c6b 100644
> > --- a/merge-ll.c
> > +++ b/merge-ll.c
> > @@ -427,8 +427,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
> >       git_check_attr(istate, path, check);
> >       ll_driver_name = check->items[0].value;
> >       if (check->items[1].value) {
> > -             marker_size = atoi(check->items[1].value);
> > -             if (marker_size <= 0)
> > +             if (strtol_i(check->items[1].value, 10, &marker_size) != 0 || marker_size <= 0)
>
> Here I think we want to return an error if we cannot parse the marker
> size and then set the default if the marker size is <= 0 like we do for
> the max_connections code in daemon.c above.
>
> >                       marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> >       }
> >       driver = find_ll_merge_driver(ll_driver_name);
> > @@ -454,8 +453,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
> >               check = attr_check_initl("conflict-marker-size", NULL);
> >       git_check_attr(istate, path, check);
> >       if (check->items[0].value) {
> > -             marker_size = atoi(check->items[0].value);
> > -             if (marker_size <= 0)
> > +             if (strtol_i(check->items[0].value, 10, &marker_size) != 0 || marker_size <= 0)
>
> And the same here
>
> Thanks for working on this, it will be a useful improvement to our
> integer parsing. I think you've got the basic idea, it just needs a bit
> of polish
>
> Phillip
>

Usman Akinyemi Oct. 17, 2024, 11:56 a.m. UTC | #24

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


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

Patrick Steinhardt Oct. 17, 2024, 12:02 p.m. UTC | #25

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

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

Patrick

Usman Akinyemi Oct. 17, 2024, 12:13 p.m. UTC | #26

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

Patch

diff --git a/daemon.c b/daemon.c
index cb946e3c95f..3fdb6e83c40 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1308,17 +1308,21 @@  int cmd_main(int argc, const char **argv)
 			continue;
 		}
 		if (skip_prefix(arg, "--timeout=", &v)) {
-			timeout = atoi(v);
+			if (strtoul_ui(v, 10, &timeout) < 0) {
+				die("'%s': not a valid integer for --timeout", v);
+			}
 			continue;
 		}
 		if (skip_prefix(arg, "--init-timeout=", &v)) {
-			init_timeout = atoi(v);
+			if (strtoul_ui(v, 10, &init_timeout) < 0) {
+				die("'%s': not a valid integer for --init-timeout", v);
+			}
 			continue;
 		}
 		if (skip_prefix(arg, "--max-connections=", &v)) {
-			max_connections = atoi(v);
-			if (max_connections < 0)
-				max_connections = 0;	        /* unlimited */
+			if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) {
+				max_connections = 0;  /* unlimited */
+			}
 			continue;
 		}
 		if (!strcmp(arg, "--strict-paths")) {
diff --git a/imap-send.c b/imap-send.c
index ec68a066877..33b74dfded7 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -668,12 +668,12 @@  static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 		return RESP_BAD;
 	}
 	if (!strcmp("UIDVALIDITY", arg)) {
-		if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
+		if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) != 0) {
 			fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
 			return RESP_BAD;
 		}
 	} else if (!strcmp("UIDNEXT", arg)) {
-		if (!(arg = next_arg(&s)) || !(imap->uidnext = atoi(arg))) {
+		if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) != 0) {
 			fprintf(stderr, "IMAP error: malformed NEXTUID status\n");
 			return RESP_BAD;
 		}
@@ -686,8 +686,8 @@  static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 		for (; isspace((unsigned char)*p); p++);
 		fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
 	} else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
-		if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
-		    !(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
+		if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) != 0) ||
+			!(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) != 0)) {
 			fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
 			return RESP_BAD;
 		}
@@ -773,7 +773,10 @@  static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			if (!tcmd)
 				return DRV_OK;
 		} else {
-			tag = atoi(arg);
+			if (strtol_i(arg, 10, &tag) != 0) {
+				fprintf(stderr, "IMAP error: malformed tag %s\n", arg);
+				return RESP_BAD;
+			}
 			for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
 				if (cmdp->tag == tag)
 					goto gottag;
diff --git a/merge-ll.c b/merge-ll.c
index 8e63071922b..2bfee0f2c6b 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -427,8 +427,7 @@  enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
 	git_check_attr(istate, path, check);
 	ll_driver_name = check->items[0].value;
 	if (check->items[1].value) {
-		marker_size = atoi(check->items[1].value);
-		if (marker_size <= 0)
+		if (strtol_i(check->items[1].value, 10, &marker_size) != 0 || marker_size <= 0)
 			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	}
 	driver = find_ll_merge_driver(ll_driver_name);
@@ -454,8 +453,7 @@  int ll_merge_marker_size(struct index_state *istate, const char *path)
 		check = attr_check_initl("conflict-marker-size", NULL);
 	git_check_attr(istate, path, check);
 	if (check->items[0].value) {
-		marker_size = atoi(check->items[0].value);
-		if (marker_size <= 0)
+		if (strtol_i(check->items[0].value, 10, &marker_size) != 0 || marker_size <= 0)
 			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	}
 	return marker_size;