| Message ID | pull.1810.v2.git.git.1729259580.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | parse: replace atoi() with strtoul_ui() and strtol_i() | expand |
On Fri, Oct 18, 2024 at 01:52:57PM +0000, Usman Akinyemi via GitGitGadget wrote: > Changes from Version 1: > > * In my initial commit, I mistakenly included changes from a different > patch and commit. This issue has now been resolved. Should we treat this as a new series, then? Or is this a true reroll of the previous round and should be kept together? > * I have split the original commit into three separate patches for better > clarity and organization. > * I added corresponding tests for each of the changes to ensure proper > functionality. > * In the first version, I used the following logic: if (strtoul_ui(v, 10, > &timeout) == 0) Based on feedback from my mentor, I improved it to: > (strtoul_ui(v, 10, &timeout)) and similar cases. > > Usman Akinyemi (3): > daemon: replace atoi() with strtoul_ui() and strtol_i() > merge: replace atoi() with strtol_i() for marker size validation > imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT > parsing Thanks, Taylor
On Fri, Oct 18, 2024 at 9:22 PM Taylor Blau <me@ttaylorr.com> wrote: > > On Fri, Oct 18, 2024 at 01:52:57PM +0000, Usman Akinyemi via GitGitGadget wrote: > > Changes from Version 1: > > > > * In my initial commit, I mistakenly included changes from a different > > patch and commit. This issue has now been resolved. > > Should we treat this as a new series, then? Or is this a true reroll of > the previous round and should be kept together? Hello Taylor, Yeah, this should be treated as a new series different from the two below. - t3404: replace test with test_line_count() - t3404: avoid losing exit status with focus on `git show` and `git cat-file` Thank you. > > > * I have split the original commit into three separate patches for better > > clarity and organization. > > * I added corresponding tests for each of the changes to ensure proper > > functionality. > > * In the first version, I used the following logic: if (strtoul_ui(v, 10, > > &timeout) == 0) Based on feedback from my mentor, I improved it to: > > (strtoul_ui(v, 10, &timeout)) and similar cases. > > > > Usman Akinyemi (3): > > daemon: replace atoi() with strtoul_ui() and strtol_i() > > merge: replace atoi() with strtol_i() for marker size validation > > imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT > > parsing > > Thanks, > Taylor
On Fri, Oct 18, 2024 at 09:29:44PM +0000, Usman Akinyemi wrote: > On Fri, Oct 18, 2024 at 9:22 PM Taylor Blau <me@ttaylorr.com> wrote: > > > > On Fri, Oct 18, 2024 at 01:52:57PM +0000, Usman Akinyemi via GitGitGadget wrote: > > > Changes from Version 1: > > > > > > * In my initial commit, I mistakenly included changes from a different > > > patch and commit. This issue has now been resolved. > > > > Should we treat this as a new series, then? Or is this a true reroll of > > the previous round and should be kept together? > Hello Taylor, > Yeah, this should be treated as a new series different from the two below. > - t3404: replace test with test_line_count() > - t3404: avoid losing exit status with focus on `git show` and `git cat-file` Gotcha. So in the original ua/t3404-cleanup series from my tree, I should drop the third and final patch: parse: replace atoi() with strtoul_ui() and strtol_i() ? Thanks, Taylor
On Fri, Oct 18, 2024 at 9:36 PM Taylor Blau <me@ttaylorr.com> wrote: > > On Fri, Oct 18, 2024 at 09:29:44PM +0000, Usman Akinyemi wrote: > > On Fri, Oct 18, 2024 at 9:22 PM Taylor Blau <me@ttaylorr.com> wrote: > > > > > > On Fri, Oct 18, 2024 at 01:52:57PM +0000, Usman Akinyemi via GitGitGadget wrote: > > > > Changes from Version 1: > > > > > > > > * In my initial commit, I mistakenly included changes from a different > > > > patch and commit. This issue has now been resolved. > > > > > > Should we treat this as a new series, then? Or is this a true reroll of > > > the previous round and should be kept together? > > Hello Taylor, > > Yeah, this should be treated as a new series different from the two below. > > - t3404: replace test with test_line_count() > > - t3404: avoid losing exit status with focus on `git show` and `git cat-file` > > Gotcha. So in the original ua/t3404-cleanup series from my tree, I > should drop the third and final patch: > > parse: replace atoi() with strtoul_ui() and strtol_i() > > ? Hello Taylor, Yeah, exactly. Thank you. Usman Akinyemi. > > Thanks, > Taylor
Changes from Version 1: * In my initial commit, I mistakenly included changes from a different patch and commit. This issue has now been resolved. * I have split the original commit into three separate patches for better clarity and organization. * I added corresponding tests for each of the changes to ensure proper functionality. * In the first version, I used the following logic: if (strtoul_ui(v, 10, &timeout) == 0) Based on feedback from my mentor, I improved it to: (strtoul_ui(v, 10, &timeout)) and similar cases. Usman Akinyemi (3): daemon: replace atoi() with strtoul_ui() and strtol_i() merge: replace atoi() with strtol_i() for marker size validation imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing daemon.c | 11 +++++++---- imap-send.c | 13 ++++++++----- merge-ll.c | 6 ++++-- t/t5570-git-daemon.sh | 27 ++++++++++++++++++++++++++- t/t6406-merge-attr.sh | 7 +++++++ 5 files changed, 52 insertions(+), 12 deletions(-) base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1810%2FUnique-Usman%2Fr_atoi-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1810/Unique-Usman/r_atoi-v2 Pull-Request: https://github.com/git/git/pull/1810 Range-diff vs v1: 1: bfff7937cd2 < -: ----------- t3404: avoid losing exit status with focus on `git show` and `git cat-file` 2: e2cae7f3a51 < -: ----------- t3404: replace test with test_line_count() -: ----------- > 1: a333d8a4013 daemon: replace atoi() with strtoul_ui() and strtol_i() -: ----------- > 2: 5d58c150efb merge: replace atoi() with strtol_i() for marker size validation 3: c93bc2d81ff ! 3: c09c7b3df0d parse: replace atoi() with strtoul_ui() and strtol_i() @@ Metadata Author: Usman Akinyemi <usmanakinyemi202@gmail.com> ## Commit message ## - parse: replace atoi() with strtoul_ui() and strtol_i() + imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing - Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers - and strtol_i() for signed integers across multiple files. This change - improves error handling and prevents potential integer overflow issues. - - The following files were updated: - - daemon.c: Update parsing of --timeout, --init-timeout, and - --max-connections - - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and - tags - - merge-ll.c: Enhance parsing of marker size in ll_merge and - ll_merge_marker_size - - This change allows for better error detection when parsing integer - values from command-line arguments and IMAP responses, making the code - more robust and secure. - - This is a #leftoverbit discussed here: - https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/ + Replaced unsafe uses of atoi() with strtol_i() to improve error handling + when parsing UIDVALIDITY, UIDNEXT, and APPENDUID in IMAP commands. + Invalid values, such as those with letters, + now trigger error messages and prevent malformed status responses. Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com> - Cc: gitster@pobox.com - Cc: Patrick Steinhardt <ps@pks.im> - Cc: phillip.wood123@gmail.com - Cc: Christian Couder <christian.couder@gmail.com> - Cc: Eric Sunshine <sunshine@sunshineco.com> - Cc: Taylor Blau <me@ttaylorr.com> - - ## daemon.c ## -@@ daemon.c: int cmd_main(int argc, const char **argv) - continue; - } - if (skip_prefix(arg, "--timeout=", &v)) { -- timeout = atoi(v); -+ if (strtoul_ui(v, 10, &timeout) < 0) { -+ die("'%s': not a valid integer for --timeout", v); -+ } - continue; - } - if (skip_prefix(arg, "--init-timeout=", &v)) { -- init_timeout = atoi(v); -+ if (strtoul_ui(v, 10, &init_timeout) < 0) { -+ die("'%s': not a valid integer for --init-timeout", v); -+ } - continue; - } - if (skip_prefix(arg, "--max-connections=", &v)) { -- max_connections = atoi(v); -- if (max_connections < 0) -- max_connections = 0; /* unlimited */ -+ if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) { -+ max_connections = 0; /* unlimited */ -+ } - continue; - } - if (!strcmp(arg, "--strict-paths")) { - ## imap-send.c ## @@ imap-send.c: 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) { ++ if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity) { 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) { ++ if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) || !imap->uidnext) { fprintf(stderr, "IMAP error: malformed NEXTUID status\n"); return RESP_BAD; } @@ imap-send.c: static int parse_response_code(struct imap_store *ctx, struct imap_ } 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)) { ++ if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity) || ++ !(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) || !cb->ctx)) { fprintf(stderr, "IMAP error: malformed APPENDUID status\n"); return RESP_BAD; } @@ imap-send.c: static int get_cmd_result(struct imap_store *ctx, struct imap_cmd * return DRV_OK; } else { - tag = atoi(arg); -+ if (strtol_i(arg, 10, &tag) != 0) { ++ if (strtol_i(arg, 10, &tag)) { + fprintf(stderr, "IMAP error: malformed tag %s\n", arg); + return RESP_BAD; + } for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next) if (cmdp->tag == tag) goto gottag; - - ## merge-ll.c ## -@@ merge-ll.c: 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); -@@ merge-ll.c: 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;