On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > Hi Usman > > On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote: > > From: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxx/ > > > > Signed-off-by: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx> > > > > Cc: gitster@xxxxxxxxx > > Cc: Patrick Steinhardt <ps@xxxxxx> > > Cc: phillip.wood123@xxxxxxxxx > > Cc: Christian Couder <christian.couder@xxxxxxxxx> > > Cc: Eric Sunshine <sunshine@xxxxxxxxxxxxxx> > > Cc: Taylor Blau <me@xxxxxxxxxxxx> > > 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 >
