threads / patch / 41737

v3builtin/apply: handle parse_binary() failure

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

## tl;dr

4 messages between Mar 18, 2016 and Apr 1, 2016. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Christian Couder· Mar 18, 2016, 12:30 UTC · lore
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(+)
Show changes to builtin/apply.c +7 −0
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
Christian Couder· Mar 31, 2016, 22:26 UTC · re: Christian Couder · lore

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

On Fri, Mar 18, 2016 at 1:30 PM, Christian Couder <christian.couder@gmail.com> wrote:

Show 18 quoted lines
> 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?
Junio C Hamano· Apr 1, 2016, 17:16 UTC · re: Christian Couder · lore

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

Christian Couder <christian.couder@gmail.com> writes:
Show 22 quoted lines
> 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?

Christian Couder· Apr 1, 2016, 19:31 UTC · re: Junio C Hamano · lore

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

On Fri, Apr 1, 2016 at 1:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> 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.

← back to recent threads