{"thread":{"id":"41713","subject":"[PATCH] builtin/apply: exit when parse_binary() fails","startedAt":"2016-03-16T19:31:15Z","lastAt":"2016-03-16T20:19:21Z","messageCount":3,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280948","messageId":"1458156675-26109-1-git-send-email-chriscool@tuxfamily.org","threadId":"41713","inReplyTo":null,"subject":"[PATCH] builtin/apply: exit when parse_binary() fails","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-16T19:31:15Z","receivedAt":"2016-03-16T19:31:15Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In parse_binary() there is:\n\n\tforward = parse_binary_hunk(&buffer, &size, &status, &used);\n\tif (!forward && !status)\n\t\t/* there has to be one hunk (forward hunk) */\n\t\treturn error(_(\"unrecognized binary patch at line %d\"), linenr-1);\n\nso parse_binary() can return -1, because that's what error() returns.\n\nAlso parse_binary_hunk() sets \"status\" to -1 in case of error and\nparse_binary() does \"if (status) return status;\".\n\nIn this case parse_chunk() should just exit, rather than add -1 to the\npatchsize it computes.\n\nSigned-off-by: Christian Couder \n---\n builtin/apply.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 42c610e..18dec0f 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1872,6 +1872,11 @@ static struct fragment *parse_binary_hunk(char **buf_p,\n \treturn NULL;\n }\n \n+/*\n+ * Returns:\n+ * -1 in case of error,\n+ * the length of the parsed binary patch otherwise\n+ */\n static int parse_binary(char *buffer, unsigned long size, struct patch *patch)\n {\n \t/*\n@@ -2017,6 +2022,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)\n \t\t\tlinenr++;\n \t\t\tused = parse_binary(buffer + hd + llen,\n \t\t\t\t\t size - hd - llen, patch);\n+\t\t\tif (used < 0)\n+\t\t\t\texit(1);\n \t\t\tif (used)\n \t\t\t\tpatchsize = used + llen;\n \t\t\telse\n-- \n2.8.0.rc2.54.g810e8ee\n"},{"id":"280951","messageId":"xmqqshzq438s.fsf@gitster.mtv.corp.google.com","threadId":"41713","inReplyTo":"1458156675-26109-1-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH] builtin/apply: exit when parse_binary() fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-16T19:44:19Z","receivedAt":"2016-03-16T19:44:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> In parse_binary() there is:\n>\n> \tforward = parse_binary_hunk(&buffer, &size, &status, &used);\n> \tif (!forward && !status)\n> \t\t/* there has to be one hunk (forward hunk) */\n> \t\treturn error(_(\"unrecognized binary patch at line %d\"), linenr-1);\n>\n> so parse_binary() can return -1, because that's what error() returns.\n>\n> Also parse_binary_hunk() sets \"status\" to -1 in case of error and\n> parse_binary() does \"if (status) return status;\".\n\nAll of the above sounds sensible, and your follow-up patch expects\nparse_chunk() to return -1 on failure--it is a bit sad that you make\nparse_chunk() to directly call exit(2). In the spirit of eventually\nlibifying this codepath, shouldn't we be turning existing die() to\nan error return, instead of introducing more calls to exit(2)?\n\n\n> In this case parse_chunk() should just exit, rather than add -1 to the\n> patchsize it computes.\n\n\n> Signed-off-by: Christian Couder \n> ---\n> builtin/apply.c | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 42c610e..18dec0f 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -1872,6 +1872,11 @@ static struct fragment *parse_binary_hunk(char **buf_p,\n> \treturn NULL;\n> }\n> \n> +/*\n> + * Returns:\n> + * -1 in case of error,\n> + * the length of the parsed binary patch otherwise\n> + */\n> static int parse_binary(char *buffer, unsigned long size, struct patch *patch)\n> {\n> \t/*\n> @@ -2017,6 +2022,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)\n> \t\t\tlinenr++;\n> \t\t\tused = parse_binary(buffer + hd + llen,\n> \t\t\t\t\t size - hd - llen, patch);\n> +\t\t\tif (used < 0)\n> +\t\t\t\texit(1);\n> \t\t\tif (used)\n> \t\t\t\tpatchsize = used + llen;\n> \t\t\telse\n"},{"id":"280957","messageId":"CAP8UFD2OxW0nDfm6P4qXkJb2BORg_r=4ZS_cVtv16YnA115w9A@mail.gmail.com","threadId":"41713","inReplyTo":"xmqqshzq438s.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/apply: exit when parse_binary() fails","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-16T20:19:21Z","receivedAt":"2016-03-16T20:19:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Mar 16, 2016 at 8:44 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> In parse_binary() there is:\n>>\n>> forward = parse_binary_hunk(&buffer, &size, &status, &used);\n>> if (!forward && !status)\n>> /* there has to be one hunk (forward hunk) */\n>> return error(_(\"unrecognized binary patch at line %d\"), linenr-1);\n>>\n>> so parse_binary() can return -1, because that's what error() returns.\n>>\n>> Also parse_binary_hunk() sets \"status\" to -1 in case of error and\n>> parse_binary() does \"if (status) return status;\".\n>\n> All of the above sounds sensible, and your follow-up patch expects\n> parse_chunk() to return -1 on failure--it is a bit sad that you make\n> parse_chunk() to directly call exit(2). In the spirit of eventually\n> libifying this codepath, shouldn't we be turning existing die() to\n> an error return, instead of introducing more calls to exit(2)?\n\nYeah, I found these little bugs when working on libifying these\nfunctions. So I agree that it is sad to introduce an exit() call now\nand I am ok to make parse_chunk() return -1 instead.\n\nI will resend with that change.\n\nThanks,\nChristian.\n"}]}