diff mbox series

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

Message ID c09c7b3df0d7eac3069cee45cddc49a76da2503e.1729259580.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. 18, 2024, 1:53 p.m. UTC 
From: Usman Akinyemi <usmanakinyemi202@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>
---
 imap-send.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Patrick Steinhardt Oct. 21, 2024, 12:20 p.m. UTC | #1 
On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@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.

The line break after "letters," is a bit funny.

It would also be nice to point out why this commit doesn't add any new
tests. I guess the answer is that we don't have any tests for
git-imap-send(1) at all, which is too bad, but a fair excuse and not a
problem of your patch. So introducing such tests would be too much to
ask.

Patrick
Usman Akinyemi Oct. 21, 2024, 12:27 p.m. UTC | #2 
On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <usmanakinyemi202@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.
>
> The line break after "letters," is a bit funny.
I just noticed that I will change it.
>
> It would also be nice to point out why this commit doesn't add any new
> tests. I guess the answer is that we don't have any tests for
> git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> problem of your patch. So introducing such tests would be too much to
> ask.
I can try, but, why was it not introduced before, is there a reason ?
>
> Patrick
Patrick Steinhardt Oct. 21, 2024, 12:34 p.m. UTC | #3 
On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > From: Usman Akinyemi <usmanakinyemi202@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.
> >
> > The line break after "letters," is a bit funny.
> I just noticed that I will change it.
> >
> > It would also be nice to point out why this commit doesn't add any new
> > tests. I guess the answer is that we don't have any tests for
> > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > problem of your patch. So introducing such tests would be too much to
> > ask.
> I can try, but, why was it not introduced before, is there a reason ?

I think it's mostly that we'd have to have an IMAP server available to
test sending emails properly, so the test setup would be comparatively
involved. Nobody felt like doing that, and thus we don't have any tests
:)

Patrick
Usman Akinyemi Oct. 21, 2024, 2:38 p.m. UTC | #4 
On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <usmanakinyemi202@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.
> > >
> > > The line break after "letters," is a bit funny.
> > I just noticed that I will change it.
> > >
> > > It would also be nice to point out why this commit doesn't add any new
> > > tests. I guess the answer is that we don't have any tests for
> > > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > > problem of your patch. So introducing such tests would be too much to
> > > ask.
> > I can try, but, why was it not introduced before, is there a reason ?
>
> I think it's mostly that we'd have to have an IMAP server available to
> test sending emails properly, so the test setup would be comparatively
> involved. Nobody felt like doing that, and thus we don't have any tests
> :)
Ohh, I see. I have not set up an IMAP server before though. I can take
it up but might require some level of guidance.

Usman Akinyemi.
>
> Patrick
Taylor Blau Oct. 21, 2024, 4:35 p.m. UTC | #5 
On Mon, Oct 21, 2024 at 02:38:40PM +0000, Usman Akinyemi wrote:
> On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> > > On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> > > >
> > > > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > > From: Usman Akinyemi <usmanakinyemi202@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.
> > > >
> > > > The line break after "letters," is a bit funny.
> > > I just noticed that I will change it.
> > > >
> > > > It would also be nice to point out why this commit doesn't add any new
> > > > tests. I guess the answer is that we don't have any tests for
> > > > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > > > problem of your patch. So introducing such tests would be too much to
> > > > ask.
> > > I can try, but, why was it not introduced before, is there a reason ?
> >
> > I think it's mostly that we'd have to have an IMAP server available to
> > test sending emails properly, so the test setup would be comparatively
> > involved. Nobody felt like doing that, and thus we don't have any tests
> > :)
> Ohh, I see. I have not set up an IMAP server before though. I can take
> it up but might require some level of guidance.

I think what Patrick is saying is that it's probably not worth the
effort to do so for an automated test, especially if the code change is
trivial by comparison.

Thanks,
Taylor
Usman Akinyemi Oct. 21, 2024, 4:36 p.m. UTC | #6 
On Mon, Oct 21, 2024 at 4:35 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Oct 21, 2024 at 02:38:40PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> > > > On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> > > > >
> > > > > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > > > From: Usman Akinyemi <usmanakinyemi202@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.
> > > > >
> > > > > The line break after "letters," is a bit funny.
> > > > I just noticed that I will change it.
> > > > >
> > > > > It would also be nice to point out why this commit doesn't add any new
> > > > > tests. I guess the answer is that we don't have any tests for
> > > > > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > > > > problem of your patch. So introducing such tests would be too much to
> > > > > ask.
> > > > I can try, but, why was it not introduced before, is there a reason ?
> > >
> > > I think it's mostly that we'd have to have an IMAP server available to
> > > test sending emails properly, so the test setup would be comparatively
> > > involved. Nobody felt like doing that, and thus we don't have any tests
> > > :)
> > Ohh, I see. I have not set up an IMAP server before though. I can take
> > it up but might require some level of guidance.
>
> I think what Patrick is saying is that it's probably not worth the
> effort to do so for an automated test, especially if the code change is
> trivial by comparison.
>
> Thanks,
> Taylor
Thanks Taylor. Noted.
Usman Akinyemi Oct. 22, 2024, 1:43 p.m. UTC | #7 
On Tue, Oct 22, 2024 at 7:21 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 21, 2024 at 12:27:05PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 12:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:53:00PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <usmanakinyemi202@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.
> > >
> > > The line break after "letters," is a bit funny.
> > I just noticed that I will change it.
> > >
> > > It would also be nice to point out why this commit doesn't add any new
> > > tests. I guess the answer is that we don't have any tests for
> > > git-imap-send(1) at all, which is too bad, but a fair excuse and not a
> > > problem of your patch. So introducing such tests would be too much to
> > > ask.
> > I can try, but, why was it not introduced before, is there a reason ?
>
> I think it's mostly that we'd have to have an IMAP server available to
> test sending emails properly, so the test setup would be comparatively
> involved. Nobody felt like doing that, and thus we don't have any tests
> :)
>
> Patrick
>
I made all these changes in version 3 of the patch. Thank you.
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;