git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 02/20] setup_temporary_shallow: avoid using inactive tempfile

From
Jeff King <peff@peff.net>
Date
Sep 5, 2017, 12:14 UTC
Message-ID
<20170905121415.xo64jttpbzae7fsj@sigill.intra.peff.net>
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
Previous: Jeff KingNext: Jeff King
Message 3 of 21 in “allowing struct lock_file to be deleted”
  1. 0/20 allowing struct lock_file to be deletedJeff King, Sep 5, 2017
  2. 01/20 write_index_as_tree: cleanup tempfile on errorJeff King, Sep 5, 2017
  3. 02/20 setup_temporary_shallow: avoid using inactive tempfileJeff King, Sep 5, 2017
  4. 03/20 setup_temporary_shallow: move tempfile struct into functionJeff King, Sep 5, 2017
  5. 04/20 verify_signed_buffer: prefer close_tempfile() to close()Jeff King, Sep 5, 2017
  6. 05/20 always check return value of close_tempfileJeff King, Sep 5, 2017
  7. 06/20 tempfile: do not delete tempfile on failed closeJeff King, Sep 5, 2017
  8. 07/20 lockfile: do not rollback lock on failed closeJeff King, Sep 5, 2017
  9. 08/20 tempfile: prefer is_tempfile_active to bare accessJeff King, Sep 5, 2017
  10. 09/20 tempfile: handle NULL tempfile pointers gracefullyJeff King, Sep 5, 2017
  11. 10/20 tempfile: replace die("BUG") with BUG()Jeff King, Sep 5, 2017
  12. 11/20 tempfile: factor out activationJeff King, Sep 5, 2017
  13. 12/20 tempfile: factor out deactivationJeff King, Sep 5, 2017
  14. 13/20 tempfile: robustify cleanup handlerJeff King, Sep 5, 2017
  15. 14/20 tempfile: release deactivated strbufs instead of resettingJeff King, Sep 5, 2017
  16. 16/20 tempfile: remove deactivated list entriesJeff King, Sep 5, 2017
  17. 17/20 tempfile: auto-allocate tempfiles on heapJeff King, Sep 5, 2017
  18. 18/20 lockfile: update lifetime requirements in documentationJeff King, Sep 5, 2017
  19. 19/20 ref_lock: stop leaking lock_filesJeff King, Sep 5, 2017
  20. 20/20 stop leaking lock structs in some simple casesJeff King, Sep 5, 2017
  21. 15/20 tempfile: use list.h for linked listJeff King, Sep 5, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.