Hi Usman

On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:

From: Usman Akinyemi <redacted>

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

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

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

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

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

Great

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

Signed-off-by: Usman Akinyemi <redacted>

Cc: gitster@pobox.com
Cc: Patrick Steinhardt <redacted>
Cc: phillip.wood123@gmail.com
Cc: Christian Couder <redacted>
Cc: Eric Sunshine <redacted>
Cc: Taylor Blau <redacted>

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.

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

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

And here

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.

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.

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