threads / patch / 40912

patchverify_pack: do not ignore return value of verification function

Subject: [PATCH] verify_pack: do not ignore return value of verification function

## tl;dr

2 messages between Nov 30, 2015 and Dec 1, 2015. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

David Turner· Nov 30, 2015, 22:47 UTC · lore

In verify_pack, a caller-supplied verification function is called. The function returns an int. If that return value is non-zero, verify_pack should fail.

The only caller of verify_pack is in builtin/fsck.c, whose verify_fn returns a meaningful error code (which was then ignored). Now, fsck might return a different error code (with more detail). This would happen in the unlikely event that a commit or tree that is a valid git object but not a valid instance of its type gets into a pack.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 pack-check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to pack-check.c +1 −1

diff --git a/pack-check.c b/pack-check.c
index 63a595c..433bd86 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -126,7 +126,7 @@ static int verify_packfile(struct packed_git *p,
 				    sha1_to_hex(entries[i].sha1), p->pack_name);
 		else if (fn) {
 			int eaten = 0;
-			fn(entries[i].sha1, type, size, data, &eaten);
+			err |= fn(entries[i].sha1, type, size, data, &eaten);
 			if (eaten)
 				data = NULL;
 		}

-- 
2.4.2.736.g391bb05-twtrsrc

Jeff King· Dec 1, 2015, 23:28 UTC · re: David Turner · lore

Re: [PATCH] verify_pack: do not ignore return value of verification function

On Mon, Nov 30, 2015 at 05:47:42PM -0500, David Turner wrote:

Show 14 quoted lines

> In verify_pack, a caller-supplied verification function is called.
> The function returns an int.  If that return value is non-zero,
> verify_pack should fail.
> 
> The only caller of verify_pack is in builtin/fsck.c, whose verify_fn
> returns a meaningful error code (which was then ignored).  Now, fsck
> might return a different error code (with more detail).  This would
> happen in the unlikely event that a commit or tree that is a valid git
> object but not a valid instance of its type gets into a pack.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  pack-check.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, this looks good to me.

-Peff