diff mbox series

[v4,3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing

Message ID 8982dca646db72f903bd4c20416d6118da1c210c.1729634938.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series parse: replace atoi() with strtoul_ui() and strtol_i() | expand

Commit Message

Usman Akinyemi Oct. 22, 2024, 10:08 p.m. UTC 
From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Replace 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.
I did not add any test for this commit as we do not have any test
for git-imap-send(1) at this point.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 imap-send.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Patrick Steinhardt Oct. 23, 2024, 6:05 a.m. UTC | #1 
On Tue, Oct 22, 2024 at 10:08:57PM +0000, Usman Akinyemi via GitGitGadget wrote:
> @@ -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) || !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;
>  		}

Two last nits from my side, sorry that I didn't spot these earlier:

  - The second line is indented incorrectly. When you have a multi-line
    condition, subsequent lines should align with the opening brace like
    this:

	if (something_something ||
	    something_else)
		frobnicate();

  - The braces around `(strtol_i() || !ctx->uidvalidity)` are a bit
    confusing and unnecessary.

Other than that I'm happy with this series, thanks!

Patrick
Usman Akinyemi Oct. 23, 2024, 7:40 a.m. UTC | #2 
On Wed, Oct 23, 2024 at 6:05 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Oct 22, 2024 at 10:08:57PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > @@ -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) || !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;
> >               }
>
> Two last nits from my side, sorry that I didn't spot these earlier:
>
>   - The second line is indented incorrectly. When you have a multi-line
>     condition, subsequent lines should align with the opening brace like
>     this:
>
>         if (something_something ||
>             something_else)
>                 frobnicate();
>
>   - The braces around `(strtol_i() || !ctx->uidvalidity)` are a bit
>     confusing and unnecessary.
Thank you Patrick for bringing my attention to this.
I fixed it now.
Usman
>
> Other than that I'm happy with this series, thanks!
>
> Patrick
diff mbox series

Patch

diff --git a/imap-send.c b/imap-send.c
index ec68a066877..8214df128e5 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) || !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) || !imap->uidnext) {
 			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) || !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;
 		}
@@ -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)) {
+				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;