[PATCH v3] builtin/apply: handle parse_binary() failure

[PATCH v3] builtin/apply: handle parse_binary() failure

[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 not add -1 to the patchsize it computes.
It is better for future libification efforts to make it just return -1.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Only the title of the patch changed in this version compared to v2.

 builtin/apply.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index 42c610e..c399c97 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)
+				return -1;
 			if (used)
 				patchsize = used + llen;
 			else
-- 
2.8.0.rc2.56.gc9044db

Re: [PATCH v3] builtin/apply: handle parse_binary() failure

[Christian Couder]

On Fri, Mar 18, 2016 at 1:30 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> 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 not add -1 to the patchsize it computes.
> It is better for future libification efforts to make it just return -1.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Only the title of the patch changed in this version compared to v2.

It looks like this patch is not in pu. Maybe it has fallen through the cracks?

Re: [PATCH v3] builtin/apply: handle parse_binary() failure

[Junio C Hamano]

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

> On Fri, Mar 18, 2016 at 1:30 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> 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 not add -1 to the patchsize it computes.
>> It is better for future libification efforts to make it just return -1.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> Only the title of the patch changed in this version compared to v2.
>
> It looks like this patch is not in pu. Maybe it has fallen through the cracks?

Yup, it indeed was ignored (giving priority to work towards 2.8
during the freeze) and then was forgotten.

My comment on the first one that exited mentions "your follow-up
patch", but I cannot quite tell which one, as there was no threading
in your original patches.  Does this change need to come before
something else that I already have?

Re: [PATCH v3] builtin/apply: handle parse_binary() failure

[Christian Couder]

On Fri, Apr 1, 2016 at 1:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>>
>> It looks like this patch is not in pu. Maybe it has fallen through the cracks?
>
> Yup, it indeed was ignored (giving priority to work towards 2.8
> during the freeze) and then was forgotten.
>
> My comment on the first one that exited mentions "your follow-up
> patch", but I cannot quite tell which one, as there was no threading
> in your original patches.  Does this change need to come before
> something else that I already have?

No, I think by "your follow-up patch" you meant the other one that had
also been forgotten and that I also replied to, that is:

[PATCH] builtin/apply: free patch when parse_chunk() fails

I had first sent both of them at around the same time.

Thanks,
Christian.