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@xxxxxxxxx>
## 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@xxxxxxxxxxxxxx/
+ 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@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>
-
- ## 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;
--
gitgitgadget
