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