* [PATCH] imap-send: handle NULL return of next_arg() @ 2017-11-01 17:03 René Scharfe 2017-11-02 2:18 ` Junio C Hamano 0 siblings, 1 reply; 3+ messages in thread From: René Scharfe @ 2017-11-01 17:03 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] imap-send: handle NULL return of next_arg() 2017-11-01 17:03 [PATCH] imap-send: handle NULL return of next_arg() René Scharfe @ 2017-11-02 2:18 ` Junio C Hamano 2017-11-02 17:27 ` René Scharfe 0 siblings, 1 reply; 3+ messages in thread From: Junio C Hamano @ 2017-11-02 2:18 UTC (permalink / raw) To: René Scharfe; +Cc: Git List 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). ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] imap-send: handle NULL return of next_arg() 2017-11-02 2:18 ` Junio C Hamano @ 2017-11-02 17:27 ` René Scharfe 0 siblings, 0 replies; 3+ messages in thread From: René Scharfe @ 2017-11-02 17:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-11-02 17:27 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-01 17:03 [PATCH] imap-send: handle NULL return of next_arg() René Scharfe 2017-11-02 2:18 ` Junio C Hamano 2017-11-02 17:27 ` René Scharfe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.