From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20]) by dcvr.yhbt.net (Postfix) with ESMTP id 0EC601F54E for ; Wed, 7 Sep 2022 21:02:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229999AbiIGVCY (ORCPT ); Wed, 7 Sep 2022 17:02:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51404 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229978AbiIGVCW (ORCPT ); Wed, 7 Sep 2022 17:02:22 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BF63CB72A7 for ; Wed, 7 Sep 2022 14:02:21 -0700 (PDT) Received: (qmail 23469 invoked by uid 109); 7 Sep 2022 21:02:21 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 07 Sep 2022 21:02:21 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 14717 invoked by uid 111); 7 Sep 2022 21:02:21 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 07 Sep 2022 17:02:21 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 7 Sep 2022 17:02:20 -0400 From: Jeff King To: Junio C Hamano Cc: =?utf-8?B?56iL5rSL?= , Derrick Stolee , "git@vger.kernel.org" , =?utf-8?B?5L2V5rWp?= , Xin7 Ma =?utf-8?B?6ams6ZGr?= , =?utf-8?B?55+z5aWJ5YW1?= , =?utf-8?B?5Yeh5Yab6L6J?= , =?utf-8?B?546L5rGJ5Z+6?= Subject: Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of "want" objects Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Wed, Sep 07, 2022 at 04:36:22PM -0400, Jeff King wrote: > Is that worth having? I dunno. It's kind of brittle in that a later > change could mean we're finding the corruption elsewhere, and not > checking exactly what we think we are. OTOH, it probably doesn't hurt to > cover more cases here, and it's not a very expensive test. So here's what it would look like as a patch on top. It could also be squashed in, of course, which saves the vague reference to "a recent commit...". OTOH, it's sufficiently complicated to discuss the testing that it's nice not to overwhelm the already-long commit message of the original. ;) -- >8 -- Subject: [PATCH] t1060: check partial clone of misnamed blob A recent commit (upload-pack: skip parse-object re-hashing of "want" objects, 2022-09-06) loosened the behavior of upload-pack so that it does not verify the sha1 of objects it receives directly via "want" requests. The existing corruption tests in t1060 aren't affected by this: the corruptions are blobs reachable from commits, and the client requests the commits. The more interesting case here is a partial clone, where the client will directly ask for the corrupted blob when it does an on-demand fetch of the filtered object. And that is not covered at all, so let's add a test. It's important here that we use the "misnamed" corruption and not "bit-error". The latter is sufficiently corrupted that upload-pack cannot even figure out the type of the object, so it bails identically both before and after the recent change. But with "misnamed", with the hash-checks enabled it sees the problem (though the error messages are a bit confusing because of the inability to create a "struct object" to store the flags): error: hash mismatch d95f3ad14dee633a758d2e331151e950dd13e4ed fatal: git upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed fatal: remote error: upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed After the change to skip the hash check, the server side happily sends the bogus object, but the client correctly realizes that it did not get the necessary data: remote: Enumerating objects: 1, done. remote: Counting objects: 100% (1/1), done. remote: Total 1 (delta 0), reused 0 (delta 0), pack-reused 0 Receiving objects: 100% (1/1), 49 bytes | 49.00 KiB/s, done. fatal: bad revision 'd95f3ad14dee633a758d2e331151e950dd13e4ed' error: [...]/misnamed did not send all necessary objects which is exactly what we expect to happen. Signed-off-by: Jeff King --- t/t1060-object-corruption.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index 5b8e47e346..35261afc9d 100755 --- a/t/t1060-object-corruption.sh +++ b/t/t1060-object-corruption.sh @@ -139,4 +139,11 @@ test_expect_success 'internal tree objects are not "missing"' ' ) ' +test_expect_success 'partial clone of corrupted repository' ' + test_config -C misnamed uploadpack.allowFilter true && + git clone --no-local --no-checkout --filter=blob:none \ + misnamed corrupt-partial && \ + test_must_fail git -C corrupt-partial checkout --force +' + test_done -- 2.37.3.1138.gedcf976b26