From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 02/20] setup_temporary_shallow: avoid using inactive tempfile
Date: Tue, 5 Sep 2017 08:14:16 -0400 [thread overview]
Message-ID: <20170905121415.xo64jttpbzae7fsj@sigill.intra.peff.net> (raw)
In-Reply-To: <20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net>
When there are no shallow entries to write, we skip creating
the tempfile entirely and try to return the empty string.
But we do so by calling get_tempfile_path() on the inactive
tempfile object. This will trigger an assertion that kills
the program. The bug was introduced by 6e122b449b
(setup_temporary_shallow(): use tempfile module,
2015-08-10). But nobody seems to have noticed since then
because we do not end up calling this function at all when
there are no shallow items. In other words, this code path
is completely unexercised.
Since the tempfile object is a static global, it _is_
possible that we call the function twice, writing out
shallow info the first time and then "reusing" our tempfile
object the second time. But:
1. It seems unlikely that this was the intent, as hitting
this code path would imply somebody clearing the
shallow_info list between calls.
And if somebody _did_ call the function multiple times
without clearing the shallow_info list, we'd hit a
different BUG for trying to reuse an already-active
tempfile.
2. I verified by code inspection that the function is only
called once per program. And also replacing this code
with a BUG() and running the test suite demonstrates
that it is not triggered there.
So we could probably just replace this with an assertion and
confirm that it's never called. However, the original intent
does seem to be that you _could_ call it when the
shallow_info is empty. And that's easy enough to do; since
the return value doesn't need to point to a writable buffer,
we can just return a string literal.
Signed-off-by: Jeff King <peff@peff.net>
---
shallow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/shallow.c b/shallow.c
index f5591e56da..29194b475a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -307,7 +307,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
* is_repository_shallow() sees empty string as "no shallow
* file".
*/
- return get_tempfile_path(&temporary_shallow);
+ return "";
}
void setup_alternate_shallow(struct lock_file *shallow_lock,
--
2.14.1.721.gc5bc1565f1
next prev parent reply other threads:[~2017-09-05 12:14 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-05 12:13 [PATCH 0/20] allowing struct lock_file to be deleted Jeff King
2017-09-05 12:14 ` [PATCH 01/20] write_index_as_tree: cleanup tempfile on error Jeff King
2017-09-05 12:14 ` Jeff King [this message]
2017-09-05 12:14 ` [PATCH 03/20] setup_temporary_shallow: move tempfile struct into function Jeff King
2017-09-05 12:14 ` [PATCH 04/20] verify_signed_buffer: prefer close_tempfile() to close() Jeff King
2017-09-05 12:14 ` [PATCH 05/20] always check return value of close_tempfile Jeff King
2017-09-05 12:14 ` [PATCH 06/20] tempfile: do not delete tempfile on failed close Jeff King
2017-09-05 12:14 ` [PATCH 07/20] lockfile: do not rollback lock " Jeff King
2017-09-05 12:14 ` [PATCH 08/20] tempfile: prefer is_tempfile_active to bare access Jeff King
2017-09-05 12:14 ` [PATCH 09/20] tempfile: handle NULL tempfile pointers gracefully Jeff King
2017-09-05 12:14 ` [PATCH 10/20] tempfile: replace die("BUG") with BUG() Jeff King
2017-09-05 12:14 ` [PATCH 11/20] tempfile: factor out activation Jeff King
2017-09-05 12:14 ` [PATCH 12/20] tempfile: factor out deactivation Jeff King
2017-09-05 12:14 ` [PATCH 13/20] tempfile: robustify cleanup handler Jeff King
2017-09-05 12:14 ` [PATCH 14/20] tempfile: release deactivated strbufs instead of resetting Jeff King
2017-09-05 12:15 ` [PATCH 15/20] tempfile: use list.h for linked list Jeff King
2017-09-05 12:15 ` [PATCH 16/20] tempfile: remove deactivated list entries Jeff King
2017-09-05 12:15 ` [PATCH 17/20] tempfile: auto-allocate tempfiles on heap Jeff King
2017-09-05 12:15 ` [PATCH 18/20] lockfile: update lifetime requirements in documentation Jeff King
2017-09-05 12:15 ` [PATCH 19/20] ref_lock: stop leaking lock_files Jeff King
2017-09-05 12:15 ` [PATCH 20/20] stop leaking lock structs in some simple cases Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170905121415.xo64jttpbzae7fsj@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).