{"thread":{"id":"47094","subject":"[PATCH] imap-send: handle NULL return of next_arg()","startedAt":"2017-11-01T17:03:35Z","lastAt":"2017-11-02T17:27:21Z","messageCount":3,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"331576","messageId":"2d466b78-77fc-fa2c-c5e1-7b8737a00958@web.de","threadId":"47094","inReplyTo":null,"subject":"[PATCH] imap-send: handle NULL return of next_arg()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-01T17:03:20Z","receivedAt":"2017-11-01T17:03:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"next_arg() returns NULL if it runs out of arguments. Most call sites\nalready handle that gracefully. Check in the remaining cases as well.\nReplace the NULL pointer with an empty string at the bottom of\nget_cmd_result() -- it's nicely reported as an unexpected response a\nfew lines down. Error out explicitly at the remaining sites.\n\nSigned-off-by: Rene Scharfe \n---\n imap-send.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 8c785f3ca2..0031566309 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -693,6 +693,10 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,\n \t}\n \t*p++ = 0;\n \targ = next_arg(&s);\n+\tif (!arg) {\n+\t\tfprintf(stderr, \"IMAP error: empty response code\\n\");\n+\t\treturn RESP_BAD;\n+\t}\n \tif (!strcmp(\"UIDVALIDITY\", arg)) {\n \t\tif (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {\n \t\t\tfprintf(stderr, \"IMAP error: malformed UIDVALIDITY status\\n\");\n@@ -725,7 +729,8 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)\n {\n \tstruct imap *imap = ctx->imap;\n \tstruct imap_cmd *cmdp, **pcmdp;\n-\tchar *cmd, *arg, *arg1;\n+\tchar *cmd;\n+\tconst char *arg, *arg1;\n \tint n, resp, resp2, tag;\n \n \tfor (;;) {\n@@ -733,6 +738,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)\n \t\t\treturn RESP_BAD;\n \n \t\targ = next_arg(&cmd);\n+\t\tif (!arg) {\n+\t\t\tfprintf(stderr, \"IMAP error: empty response\\n\");\n+\t\t\treturn RESP_BAD;\n+\t\t}\n \t\tif (*arg == '*') {\n \t\t\targ = next_arg(&cmd);\n \t\t\tif (!arg) {\n@@ -807,6 +816,8 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)\n \t\t\tif (cmdp->cb.cont || cmdp->cb.data)\n \t\t\t\timap->literal_pending = 0;\n \t\t\targ = next_arg(&cmd);\n+\t\t\tif (!arg)\n+\t\t\t\targ = \"\";\n \t\t\tif (!strcmp(\"OK\", arg))\n \t\t\t\tresp = DRV_OK;\n \t\t\telse {\n-- \n2.15.0\n"},{"id":"331639","messageId":"xmqqefphs9rx.fsf@gitster.mtv.corp.google.com","threadId":"47094","inReplyTo":"2d466b78-77fc-fa2c-c5e1-7b8737a00958@web.de","subject":"Re: [PATCH] imap-send: handle NULL return of next_arg()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-02T02:18:42Z","receivedAt":"2017-11-02T02:18:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> @@ -807,6 +816,8 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)\n> \t\t\tif (cmdp->cb.cont || cmdp->cb.data)\n> \t\t\t\timap->literal_pending = 0;\n> \t\t\targ = next_arg(&cmd);\n> +\t\t\tif (!arg)\n> +\t\t\t\targ = \"\";\n\nThis is being cute and needs reading of the post-context a bit.\n\n arg = next_arg(&cmd);\n+ if (!arg)\n+ arg = \"\";\n if (!strcmp(\"OK\", arg))\n resp = DRV_OK;\n else {\n if (!strcmp(\"NO\", arg))\n resp = RESP_NO;\n else /*if (!strcmp(\"BAD\", arg))*/\n resp = RESP_BAD;\n fprintf(stderr, \"IMAP command '%s' returned response (%s) - %s\\n\",\n !starts_with(cmdp->cmd, \"LOGIN\") ?\n cmdp->cmd : \"LOGIN \",\n arg, cmd ? cmd : \"\");\n }\n if ((resp2 = parse_response_code(ctx, &cmdp->cb, cmd)) > resp)\n resp = resp2;\n\n\nBecause the existing code does not explicitly check for \"BAD\", we\nget RESP_BAD, which is what we want in the end, and the error\nmessage we give has \"returned response (%s)\" which is filled with\nthis empty string.\n\nI notice that when this change matters, i.e. we hit a premature end,\nnext_arg() that yielded NULL would have cleared cmd. That is OK for\nthe fprintf() we see above, but wouldn't it hit parse_response_code()\nthat is shared between good and bad cases? The function starts like\nso:\n\n static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,\n char *s)\n {\n struct imap *imap = ctx->imap;\n char *arg, *p;\n\n if (*s != '[')\n return RESP_OK;\t\t/* no response code */\n\nso we will get an immediate NULL dereference, it appears.\n\nThe fixes in other hunks looked OK (and not cute).\n\n"},{"id":"331672","messageId":"e71377c8-ce87-c7f4-138a-41dc1cc6f3dc@web.de","threadId":"47094","inReplyTo":"xmqqefphs9rx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] imap-send: handle NULL return of next_arg()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-02T17:27:05Z","receivedAt":"2017-11-02T17:27:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.11.2017 um 03:18 schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> @@ -807,6 +816,8 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)\n>> \t\t\tif (cmdp->cb.cont || cmdp->cb.data)\n>> \t\t\t\timap->literal_pending = 0;\n>> \t\t\targ = next_arg(&cmd);\n>> +\t\t\tif (!arg)\n>> +\t\t\t\targ = \"\";\n> \n> This is being cute and needs reading of the post-context a bit.\n> \n> arg = next_arg(&cmd);\n> + if (!arg)\n> + arg = \"\";\n> if (!strcmp(\"OK\", arg))\n> resp = DRV_OK;\n> else {\n> if (!strcmp(\"NO\", arg))\n> resp = RESP_NO;\n> else /*if (!strcmp(\"BAD\", arg))*/\n> resp = RESP_BAD;\n> fprintf(stderr, \"IMAP command '%s' returned response (%s) - %s\\n\",\n> !starts_with(cmdp->cmd, \"LOGIN\") ?\n> cmdp->cmd : \"LOGIN \",\n> arg, cmd ? cmd : \"\");\n> }\n> if ((resp2 = parse_response_code(ctx, &cmdp->cb, cmd)) > resp)\n> resp = resp2;\n> \n> \n> Because the existing code does not explicitly check for \"BAD\", we\n> get RESP_BAD, which is what we want in the end, and the error\n> message we give has \"returned response (%s)\" which is filled with\n> this empty string.\n> \n> I notice that when this change matters, i.e. we hit a premature end,\n> next_arg() that yielded NULL would have cleared cmd. That is OK for\n> the fprintf() we see above, but wouldn't it hit parse_response_code()\n> that is shared between good and bad cases? The function starts like\n> so:\n> \n> static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,\n> char *s)\n> {\n> struct imap *imap = ctx->imap;\n> char *arg, *p;\n> \n> if (*s != '[')\n> return RESP_OK;\t\t/* no response code */\n> \n> so we will get an immediate NULL dereference, it appears.\n\nGood catch.\n\nThe NULL check in the fprintf() call (two lines above) makes it obvious\nalready that cmd can be NULL. So parse_response_code() needs to learn\nto handle NULL passed as its third parameter. And since \"no response\ncode\" yields \"RESP_OK\" I guess that this should be an appropriate\nreaction to s == NULL as well.\n\nRFC 3501 seems to agree (response codes are optional):\n\n 7.1. Server Responses - Status Responses\n\n Status responses are OK, NO, BAD, PREAUTH and BYE. OK, NO, and BAD\n can be tagged or untagged. PREAUTH and BYE are always untagged.\n\n Status responses MAY include an OPTIONAL \"response code\". A response\n code consists of data inside square brackets in the form of an atom,\n possibly followed by a space and arguments. The response code\n contains additional information or status codes for client software\n beyond the OK/NO/BAD condition, and are defined when there is a\n specific action that a client can take based upon the additional\n information.\n\nThe follow-up patch below makes sense in this light, but I wonder why\nit hasn't been necessary so far. Do most IMAP servers actually send\nresponse codes? Or at least some other string after a status response?\nAre no users of git imap-send left? I have no way to test any of that.\n:-(\n\nRené\n\n-- >8 --\nSubject: [PATCH 2/1] imap-send: handle missing response codes gracefully\n\nResponse codes are optional. Exit parse_response_code() early if it's\npassed a NULL string, indicating that we reached the end of the reply.\nThis avoids dereferencing said NULL pointer.\n\nNoticed-by: Junio C Hamano \nSigned-off-by: Rene Scharfe \n---\n imap-send.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 0031566309..12cc4ea4c8 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -684,7 +684,7 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,\n \tstruct imap *imap = ctx->imap;\n \tchar *arg, *p;\n \n-\tif (*s != '[')\n+\tif (!s || *s != '[')\n \t\treturn RESP_OK;\t\t/* no response code */\n \ts++;\n \tif (!(p = strchr(s, ']'))) {\n-- \n2.15.0\n"}]}