[PATCH] builtin/apply: exit when parse_binary() fails

[PATCH] builtin/apply: exit when parse_binary() fails

[Christian Couder]

In parse_binary() there is:

	forward = parse_binary_hunk(&buffer, &size, &status, &used);
	if (!forward && !status)
		/* there has to be one hunk (forward hunk) */
		return error(_("unrecognized binary patch at line %d"), linenr-1);

so parse_binary() can return -1, because that's what error() returns.

Also parse_binary_hunk() sets "status" to -1 in case of error and
parse_binary() does "if (status) return status;".

In this case parse_chunk() should just exit, rather than add -1 to the
patchsize it computes.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/apply.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index 42c610e..18dec0f 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1872,6 +1872,11 @@ static struct fragment *parse_binary_hunk(char **buf_p,
 	return NULL;
 }
 
+/*
+ * Returns:
+ *   -1 in case of error,
+ *   the length of the parsed binary patch otherwise
+ */
 static int parse_binary(char *buffer, unsigned long size, struct patch *patch)
 {
 	/*
@@ -2017,6 +2022,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)
 			linenr++;
 			used = parse_binary(buffer + hd + llen,
 					    size - hd - llen, patch);
+			if (used < 0)
+				exit(1);
 			if (used)
 				patchsize = used + llen;
 			else
-- 
2.8.0.rc2.54.g810e8ee

Re: [PATCH] builtin/apply: exit when parse_binary() fails

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> In parse_binary() there is:
>
> 	forward = parse_binary_hunk(&buffer, &size, &status, &used);
> 	if (!forward && !status)
> 		/* there has to be one hunk (forward hunk) */
> 		return error(_("unrecognized binary patch at line %d"), linenr-1);
>
> so parse_binary() can return -1, because that's what error() returns.
>
> Also parse_binary_hunk() sets "status" to -1 in case of error and
> parse_binary() does "if (status) return status;".

All of the above sounds sensible, and your follow-up patch expects
parse_chunk() to return -1 on failure--it is a bit sad that you make
parse_chunk() to directly call exit(2).  In the spirit of eventually
libifying this codepath, shouldn't we be turning existing die() to
an error return, instead of introducing more calls to exit(2)?


> In this case parse_chunk() should just exit, rather than add -1 to the
> patchsize it computes.


> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/apply.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 42c610e..18dec0f 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -1872,6 +1872,11 @@ static struct fragment *parse_binary_hunk(char **buf_p,
>  	return NULL;
>  }
>  
> +/*
> + * Returns:
> + *   -1 in case of error,
> + *   the length of the parsed binary patch otherwise
> + */
>  static int parse_binary(char *buffer, unsigned long size, struct patch *patch)
>  {
>  	/*
> @@ -2017,6 +2022,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)
>  			linenr++;
>  			used = parse_binary(buffer + hd + llen,
>  					    size - hd - llen, patch);
> +			if (used < 0)
> +				exit(1);
>  			if (used)
>  				patchsize = used + llen;
>  			else

Re: [PATCH] builtin/apply: exit when parse_binary() fails

[Christian Couder]

On Wed, Mar 16, 2016 at 8:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> In parse_binary() there is:
>>
>>       forward = parse_binary_hunk(&buffer, &size, &status, &used);
>>       if (!forward && !status)
>>               /* there has to be one hunk (forward hunk) */
>>               return error(_("unrecognized binary patch at line %d"), linenr-1);
>>
>> so parse_binary() can return -1, because that's what error() returns.
>>
>> Also parse_binary_hunk() sets "status" to -1 in case of error and
>> parse_binary() does "if (status) return status;".
>
> All of the above sounds sensible, and your follow-up patch expects
> parse_chunk() to return -1 on failure--it is a bit sad that you make
> parse_chunk() to directly call exit(2).  In the spirit of eventually
> libifying this codepath, shouldn't we be turning existing die() to
> an error return, instead of introducing more calls to exit(2)?

Yeah, I found these little bugs when working on libifying these
functions. So I agree that it is sad to introduce an exit() call now
and I am ok to make parse_chunk() return -1 instead.

I will resend with that change.

Thanks,
Christian.