From patchwork Tue Oct 30 23:18:51 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10661819 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 0E25F1751 for ; Tue, 30 Oct 2018 23:18:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F10682A96F for ; Tue, 30 Oct 2018 23:18:54 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E48B22A978; Tue, 30 Oct 2018 23:18:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8C6EA2A96F for ; Tue, 30 Oct 2018 23:18:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728680AbeJaIOZ (ORCPT ); Wed, 31 Oct 2018 04:14:25 -0400 Received: from cloud.peff.net ([104.130.231.41]:33516 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726376AbeJaIOY (ORCPT ); Wed, 31 Oct 2018 04:14:24 -0400 Received: (qmail 28949 invoked by uid 109); 30 Oct 2018 23:18:53 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 30 Oct 2018 23:18:53 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 5328 invoked by uid 111); 30 Oct 2018 23:18:08 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Tue, 30 Oct 2018 19:18:08 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 30 Oct 2018 19:18:51 -0400 Date: Tue, 30 Oct 2018 19:18:51 -0400 From: Jeff King To: Junio C Hamano Cc: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason , Git Mailing List , John Szakmeister , Dennis Kaarsemaker Subject: [PATCH 1/3] t1450: check large blob in trailing-garbage test Message-ID: <20181030231850.GA32038@sigill.intra.peff.net> References: <20181030231232.GA6141@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181030231232.GA6141@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Commit cce044df7f (fsck: detect trailing garbage in all object types, 2017-01-13) added two tests of trailing garbage in a loose object file: one with a commit and one with a blob. The point of having two is that blobs would follow a different code path that streamed the contents, instead of loading it into a buffer as usual. At the time, merely being a blob was enough to trigger the streaming code path. But since 7ac4f3a007 (fsck: actually fsck blob data, 2018-05-02), we now only stream blobs that are actually large. So since then, the streaming code path is not tested at all for this case. We can restore the original intent of the test by tweaking core.bigFileThreshold to make our small blob seem large. There's no easy way to externally verify that we followed the streaming code path, but I did check before/after using a temporary debug statement. Signed-off-by: Jeff King --- I prepared this series on master, but it occurs to me you may want to apply patch 2 on top of f6371f9210 or thereabouts, which introduced the bug it fixes. If so, then obviously this one doesn't make sense back then, and should go on top of 7ac4f3a007. It should be semantically independent, though there may be a minor text conflict. t/t1450-fsck.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 0f2dd26f74..3421f12e8a 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -673,13 +673,13 @@ test_expect_success 'fsck detects trailing loose garbage (commit)' ' test_i18ngrep "garbage.*$commit" out ' -test_expect_success 'fsck detects trailing loose garbage (blob)' ' +test_expect_success 'fsck detects trailing loose garbage (large blob)' ' blob=$(echo trailing | git hash-object -w --stdin) && file=$(sha1_file $blob) && test_when_finished "remove_object $blob" && chmod +w "$file" && echo garbage >>"$file" && - test_must_fail git fsck 2>out && + test_must_fail git -c core.bigfilethreshold=5 fsck 2>out && test_i18ngrep "garbage.*$blob" out ' From patchwork Tue Oct 30 23:23:12 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10661825 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 088F514DE for ; Tue, 30 Oct 2018 23:50:00 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E0DF82A6A3 for ; Tue, 30 Oct 2018 23:49:59 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CFD392A6A2; Tue, 30 Oct 2018 23:49:59 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1C5E92A6A2 for ; Tue, 30 Oct 2018 23:49:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728225AbeJaIpe (ORCPT ); Wed, 31 Oct 2018 04:45:34 -0400 Received: from cloud.peff.net ([104.130.231.41]:33568 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726366AbeJaIpd (ORCPT ); Wed, 31 Oct 2018 04:45:33 -0400 Received: (qmail 29112 invoked by uid 109); 30 Oct 2018 23:23:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 30 Oct 2018 23:23:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 5429 invoked by uid 111); 30 Oct 2018 23:22:30 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Tue, 30 Oct 2018 19:22:30 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 30 Oct 2018 19:23:12 -0400 Date: Tue, 30 Oct 2018 19:23:12 -0400 From: Jeff King To: Junio C Hamano Cc: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason , Git Mailing List , John Szakmeister , Dennis Kaarsemaker Subject: [PATCH 2/3] check_stream_sha1(): handle input underflow Message-ID: <20181030232312.GB32038@sigill.intra.peff.net> References: <20181030231232.GA6141@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181030231232.GA6141@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP This commit fixes an infinite loop when fscking large truncated loose objects. The check_stream_sha1() function takes an mmap'd loose object buffer and streams 4k of output at a time, checking its sha1. The loop quits when we've output enough bytes (we know the size from the object header), or when zlib tells us anything except Z_OK or Z_BUF_ERROR. The latter is expected because zlib may run out of room in our 4k buffer, and that is how it tells us to process the output and loop again. But Z_BUF_ERROR also covers another case: one in which zlib cannot make forward progress because it needs more _input_. This should never happen in this loop, because though we're streaming the output, we have the entire deflated input available in the mmap'd buffer. But since we don't check this case, we'll just loop infinitely if we do see a truncated object, thinking that zlib is asking for more output space. It's tempting to fix this by checking stream->avail_in as part of the loop condition (and quitting if all of our bytes have been consumed). But that assumes that once zlib has consumed the input, there is nothing left to do. That's not necessarily the case: it may have read our input into its internal state, but still have bytes to output. Instead, let's continue on Z_BUF_ERROR only when we see the case we're expecting: the previous round filled our output buffer completely. If it didn't (and we still saw Z_BUF_ERROR), we know something is wrong and should break out of the loop. The bug comes from commit f6371f9210 (sha1_file: add read_loose_object() function, 2017-01-13), which reimplemented some of the existing loose object functions. So it's worth checking if this bug was inherited from any of those. The answers seems to be no. The two obvious candidates are both OK: 1. unpack_sha1_rest(); this doesn't need to loop on Z_BUF_ERROR at all, since it allocates the expected output buffer in advance (which we can't do since we're explicitly streaming here) 2. check_object_signature(); the streaming path relies on the istream interface, which uses read_istream_loose() for this case. That function uses a similar "is our output buffer full" check with Z_BUF_ERROR (which is where I stole it from for this patch!) Reported-by: Ævar Arnfjörð Bjarmason Helped-by: Junio C Hamano Signed-off-by: Jeff King Signed-off-by: Jeff King --- sha1-file.c | 3 ++- t/t1450-fsck.sh | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/sha1-file.c b/sha1-file.c index dd0b6aa873..2daf7d9935 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -2199,7 +2199,8 @@ static int check_stream_sha1(git_zstream *stream, * see the comment in unpack_sha1_rest for details. */ while (total_read <= size && - (status == Z_OK || status == Z_BUF_ERROR)) { + (status == Z_OK || + (status == Z_BUF_ERROR && !stream->avail_out))) { stream->next_out = buf; stream->avail_out = sizeof(buf); if (size - total_read < stream->avail_out) diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 3421f12e8a..b5677d26a4 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -683,6 +683,25 @@ test_expect_success 'fsck detects trailing loose garbage (large blob)' ' test_i18ngrep "garbage.*$blob" out ' +test_expect_success 'fsck detects truncated loose object' ' + # make it big enough that we know we will truncate in the data + # portion, not the header + test-tool genrandom truncate 4096 >file && + blob=$(git hash-object -w file) && + file=$(sha1_file $blob) && + test_when_finished "remove_object $blob" && + test_copy_bytes 1024 <"$file" >tmp && + rm "$file" && + mv -f tmp "$file" && + + # check both regular and streaming code paths + test_must_fail git fsck 2>out && + test_i18ngrep corrupt.*$blob out && + + test_must_fail git -c core.bigfilethreshold=128 fsck 2>out && + test_i18ngrep corrupt.*$blob out +' + # for each of type, we have one version which is referenced by another object # (and so while unreachable, not dangling), and another variant which really is # dangling. From patchwork Tue Oct 30 23:23:38 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10661827 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 8667614DE for ; Tue, 30 Oct 2018 23:50:22 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 765032A6A2 for ; Tue, 30 Oct 2018 23:50:22 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 684062A6AD; Tue, 30 Oct 2018 23:50:22 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0B5922A6A2 for ; Tue, 30 Oct 2018 23:50:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728586AbeJaIp6 (ORCPT ); Wed, 31 Oct 2018 04:45:58 -0400 Received: from cloud.peff.net ([104.130.231.41]:33574 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726366AbeJaIp5 (ORCPT ); Wed, 31 Oct 2018 04:45:57 -0400 Received: (qmail 29127 invoked by uid 109); 30 Oct 2018 23:23:40 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 30 Oct 2018 23:23:40 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 5436 invoked by uid 111); 30 Oct 2018 23:22:55 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Tue, 30 Oct 2018 19:22:55 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 30 Oct 2018 19:23:38 -0400 Date: Tue, 30 Oct 2018 19:23:38 -0400 From: Jeff King To: Junio C Hamano Cc: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason , Git Mailing List , John Szakmeister , Dennis Kaarsemaker Subject: [PATCH 3/3] cat-file: handle streaming failures consistently Message-ID: <20181030232337.GC32038@sigill.intra.peff.net> References: <20181030231232.GA6141@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181030231232.GA6141@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP There are three ways to convince cat-file to stream a blob: - cat-file -p $blob - cat-file blob $blob - echo $batch | cat-file --batch In the first two, we simply exit with the error code of streaw_blob_to_fd(). That means that an error will cause us to exit with "-1" (which we try to avoid) without printing any kind of error message (which is confusing to the user). Instead, let's match the third case, which calls die() on an error. Unfortunately we cannot be more specific, as stream_blob_to_fd() does not tell us whether the problem was on reading (e.g., a corrupt object) or on writing (e.g., ENOSPC). That might be an opportunity for future work, but for now we will at least exit with a sane message and exit code. Signed-off-by: Jeff King --- builtin/cat-file.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 8d97c84725..0d403eb77d 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -50,6 +50,13 @@ static int filter_object(const char *path, unsigned mode, return 0; } +static int stream_blob(const struct object_id *oid) +{ + if (stream_blob_to_fd(1, oid, NULL, 0)) + die("unable to stream %s to stdout", oid_to_hex(oid)); + return 0; +} + static int cat_one_file(int opt, const char *exp_type, const char *obj_name, int unknown_type) { @@ -132,7 +139,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, } if (type == OBJ_BLOB) - return stream_blob_to_fd(1, &oid, NULL, 0); + return stream_blob(&oid); buf = read_object_file(&oid, &type, &size); if (!buf) die("Cannot read object %s", obj_name); @@ -155,7 +162,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, oidcpy(&blob_oid, &oid); if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB) - return stream_blob_to_fd(1, &blob_oid, NULL, 0); + return stream_blob(&blob_oid); /* * we attempted to dereference a tag to a blob * and failed; there may be new dereference @@ -319,8 +326,9 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d BUG("invalid cmdmode: %c", opt->cmdmode); batch_write(opt, contents, size); free(contents); - } else if (stream_blob_to_fd(1, oid, NULL, 0) < 0) - die("unable to stream %s to stdout", oid_to_hex(oid)); + } else { + stream_blob(oid); + } } else { enum object_type type;