[PATCH] imap-send: handle NULL return of next_arg()

[PATCH] imap-send: handle NULL return of next_arg()

[René Scharfe]

next_arg() returns NULL if it runs out of arguments.  Most call sites
already handle that gracefully.  Check in the remaining cases as well.
Replace the NULL pointer with an empty string at the bottom of
get_cmd_result() -- it's nicely reported as an unexpected response a
few lines down.  Error out explicitly at the remaining sites.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 imap-send.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index 8c785f3ca2..0031566309 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -693,6 +693,10 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 	}
 	*p++ = 0;
 	arg = next_arg(&s);
+	if (!arg) {
+		fprintf(stderr, "IMAP error: empty response code\n");
+		return RESP_BAD;
+	}
 	if (!strcmp("UIDVALIDITY", arg)) {
 		if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
 			fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
@@ -725,7 +729,8 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 {
 	struct imap *imap = ctx->imap;
 	struct imap_cmd *cmdp, **pcmdp;
-	char *cmd, *arg, *arg1;
+	char *cmd;
+	const char *arg, *arg1;
 	int n, resp, resp2, tag;
 
 	for (;;) {
@@ -733,6 +738,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			return RESP_BAD;
 
 		arg = next_arg(&cmd);
+		if (!arg) {
+			fprintf(stderr, "IMAP error: empty response\n");
+			return RESP_BAD;
+		}
 		if (*arg == '*') {
 			arg = next_arg(&cmd);
 			if (!arg) {
@@ -807,6 +816,8 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			if (cmdp->cb.cont || cmdp->cb.data)
 				imap->literal_pending = 0;
 			arg = next_arg(&cmd);
+			if (!arg)
+				arg = "";
 			if (!strcmp("OK", arg))
 				resp = DRV_OK;
 			else {
-- 
2.15.0

Re: [PATCH] imap-send: handle NULL return of next_arg()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> @@ -807,6 +816,8 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
>  			if (cmdp->cb.cont || cmdp->cb.data)
>  				imap->literal_pending = 0;
>  			arg = next_arg(&cmd);
> +			if (!arg)
> +				arg = "";

This is being cute and needs reading of the post-context a bit.

         arg = next_arg(&cmd);
+        if (!arg)
+                arg = "";
         if (!strcmp("OK", arg))
                 resp = DRV_OK;
         else {
                 if (!strcmp("NO", arg))
                         resp = RESP_NO;
                 else /*if (!strcmp("BAD", arg))*/
                         resp = RESP_BAD;
                 fprintf(stderr, "IMAP command '%s' returned response (%s) - %s\n",
                         !starts_with(cmdp->cmd, "LOGIN") ?
                                         cmdp->cmd : "LOGIN <user> <pass>",
                                         arg, cmd ? cmd : "");
         }
         if ((resp2 = parse_response_code(ctx, &cmdp->cb, cmd)) > resp)
                 resp = resp2;


Because the existing code does not explicitly check for "BAD", we
get RESP_BAD, which is what we want in the end, and the error
message we give has "returned response (%s)" which is filled with
this empty string.

I notice that when this change matters, i.e. we hit a premature end,
next_arg() that yielded NULL would have cleared cmd.  That is OK for
the fprintf() we see above, but wouldn't it hit parse_response_code()
that is shared between good and bad cases?  The function starts like
so:

    static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
                                   char *s)
    {
            struct imap *imap = ctx->imap;
            char *arg, *p;

            if (*s != '[')
                    return RESP_OK;		/* no response code */

so we will get an immediate NULL dereference, it appears.

The fixes in other hunks looked OK (and not cute).

Re: [PATCH] imap-send: handle NULL return of next_arg()

[René Scharfe]

Am 02.11.2017 um 03:18 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
> 
>> @@ -807,6 +816,8 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
>>   			if (cmdp->cb.cont || cmdp->cb.data)
>>   				imap->literal_pending = 0;
>>   			arg = next_arg(&cmd);
>> +			if (!arg)
>> +				arg = "";
> 
> This is being cute and needs reading of the post-context a bit.
> 
>           arg = next_arg(&cmd);
> +        if (!arg)
> +                arg = "";
>           if (!strcmp("OK", arg))
>                   resp = DRV_OK;
>           else {
>                   if (!strcmp("NO", arg))
>                           resp = RESP_NO;
>                   else /*if (!strcmp("BAD", arg))*/
>                           resp = RESP_BAD;
>                   fprintf(stderr, "IMAP command '%s' returned response (%s) - %s\n",
>                           !starts_with(cmdp->cmd, "LOGIN") ?
>                                           cmdp->cmd : "LOGIN <user> <pass>",
>                                           arg, cmd ? cmd : "");
>           }
>           if ((resp2 = parse_response_code(ctx, &cmdp->cb, cmd)) > resp)
>                   resp = resp2;
> 
> 
> Because the existing code does not explicitly check for "BAD", we
> get RESP_BAD, which is what we want in the end, and the error
> message we give has "returned response (%s)" which is filled with
> this empty string.
> 
> I notice that when this change matters, i.e. we hit a premature end,
> next_arg() that yielded NULL would have cleared cmd.  That is OK for
> the fprintf() we see above, but wouldn't it hit parse_response_code()
> that is shared between good and bad cases?  The function starts like
> so:
> 
>      static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
>                                     char *s)
>      {
>              struct imap *imap = ctx->imap;
>              char *arg, *p;
> 
>              if (*s != '[')
>                      return RESP_OK;		/* no response code */
> 
> so we will get an immediate NULL dereference, it appears.

Good catch.

The NULL check in the fprintf() call (two lines above) makes it obvious
already that cmd can be NULL.  So parse_response_code() needs to learn
to handle NULL passed as its third parameter.  And since "no response
code" yields "RESP_OK" I guess that this should be an appropriate
reaction to s == NULL as well.

RFC 3501 seems to agree (response codes are optional):

   7.1.    Server Responses - Status Responses

   Status responses are OK, NO, BAD, PREAUTH and BYE.  OK, NO, and BAD
   can be tagged or untagged.  PREAUTH and BYE are always untagged.

   Status responses MAY include an OPTIONAL "response code".  A response
   code consists of data inside square brackets in the form of an atom,
   possibly followed by a space and arguments.  The response code
   contains additional information or status codes for client software
   beyond the OK/NO/BAD condition, and are defined when there is a
   specific action that a client can take based upon the additional
   information.

The follow-up patch below makes sense in this light, but I wonder why
it hasn't been necessary so far.  Do most IMAP servers actually send
response codes?  Or at least some other string after a status response?
Are no users of git imap-send left?  I have no way to test any of that.
:-(

René

-- >8 --
Subject: [PATCH 2/1] imap-send: handle missing response codes gracefully

Response codes are optional.  Exit parse_response_code() early if it's
passed a NULL string, indicating that we reached the end of the reply.
This avoids dereferencing said NULL pointer.

Noticed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 imap-send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index 0031566309..12cc4ea4c8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -684,7 +684,7 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 	struct imap *imap = ctx->imap;
 	char *arg, *p;
 
-	if (*s != '[')
+	if (!s || *s != '[')
 		return RESP_OK;		/* no response code */
 	s++;
 	if (!(p = strchr(s, ']'))) {
-- 
2.15.0