From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Michael Haggerty <mhagger@alum.mit.edu>
Subject: [PATCH 0/17] removing questionable uses of git_path
Date: Mon, 10 Aug 2015 05:27:32 -0400 [thread overview]
Message-ID: <20150810092731.GA9027@sigill.intra.peff.net> (raw)
Recently Michael and I were working on a patch series (not yet
published), which did something like:
const char *path = git_path("foo");
... do stuff with path ...
for_each_ref(some_callback, NULL);
... do some other stuff ...
unlink(path);
Clever readers may have spotted the bug immediately, but we did not,
until we found that random loose refs were being deleted from the
repository.
The problem is that git_path uses a static buffer that gets overwritten
by subsequent calls. The ref code uses it to iterate over all of the
loose refs in a directory, so our original path is trashed before
for_each_ref returns. Except to make it even more exciting, git_path
actually has a ring of _four_ buffers, so any trivial test you write
will probably work just fine; it's only when you use a real repository
that it causes problems (and then, only if the code path is such that
the loose refs were not previously accessed and cached!).
Michael likened git_path to "a hand-grenade with the pin pulled out",
and I tend to agree. On the other hand, it's pretty darn useful to be
able to get a quick path without having to deal with memory allocation
and ownership. This patch series tries to document the danger, and
remove some of the more questionable uses. I don't know whether this is
fixing any actual latent bugs; I traced a number of the code paths
manually, but never found a bug. There were some near misses, though,
which make me believe that seemingly-unrelated refactoring could
introduce a bug.
I stopped short of trying to eradicate git_path entirely, and settled
for:
git grep -E '[^_](git_|mk)path\('
producing a fairly tame-looking set of function calls. It's OK to pass
the result of git_path() to a system call, or something that is a thin
wrapper around one (e.g., strbuf_read_file).
I think this takes us most of the way there. I left out a few cases
where introducing allocations would have been awkward, and I verified
that there were no bugs (e.g., rerere_path). And I left out a few spots
that conflict with topics in "next" (and luckily, in all cases what is
in next makes the problem go away, so we do not have to follow-up for
those sites).
Along the way, there are a few cleanups (e.g., I polished off the recent
hold_lock_file_for_append topic which was on the list, as it had some
problematic calls).
[01/17]: cache.h: clarify documentation for git_path, et al
[02/17]: cache.h: complete set of git_path_submodule helpers
[03/17]: t5700: modernize style
[04/17]: add_to_alternates_file: don't add duplicate entries
[05/17]: remove hold_lock_file_for_append
[06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases
[07/17]: prefer mkpathdup to mkpath in assignments
[08/17]: remote.c: drop extraneous local variable from migrate_file
[09/17]: refs.c: remove extra git_path calls from read_loose refs
[10/17]: path.c: drop git_path_submodule
[11/17]: refs.c: simplify strbufs in reflog setup and writing
[12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log
[13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic
[14/17]: refs.c: remove_empty_directories can take a strbuf
[15/17]: find_hook: keep our own static buffer
[16/17]: get_repo_path: refactor path-allocation
[17/17]: memoize common git-path "constant" files
-Peff
next reply other threads:[~2015-08-10 9:27 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-10 9:27 Jeff King [this message]
2015-08-10 9:32 ` [PATCH 01/17] cache.h: clarify documentation for git_path, et al Jeff King
2015-08-10 9:32 ` [PATCH 02/17] cache.h: complete set of git_path_submodule helpers Jeff King
2015-08-10 9:32 ` [PATCH 03/17] t5700: modernize style Jeff King
2015-08-10 9:34 ` [PATCH 04/17] add_to_alternates_file: don't add duplicate entries Jeff King
2015-08-11 4:00 ` Michael Haggerty
2015-08-11 9:54 ` Jeff King
2015-08-10 9:35 ` [PATCH 05/17] remove hold_lock_file_for_append Jeff King
2015-08-10 22:36 ` Junio C Hamano
2015-08-11 9:38 ` Jeff King
2015-08-10 9:35 ` [PATCH 06/17] prefer git_pathdup to git_path in some possibly-dangerous cases Jeff King
2015-08-10 9:35 ` [PATCH 07/17] prefer mkpathdup to mkpath in assignments Jeff King
2015-08-10 9:35 ` [PATCH 08/17] remote.c: drop extraneous local variable from migrate_file Jeff King
2015-08-10 9:36 ` [PATCH 09/17] refs.c: remove extra git_path calls from read_loose_refs Jeff King
2015-08-10 9:36 ` [PATCH 10/17] path.c: drop git_path_submodule Jeff King
2015-08-10 22:50 ` Junio C Hamano
2015-08-10 22:57 ` Junio C Hamano
2015-08-10 23:52 ` Junio C Hamano
2015-08-11 9:53 ` Jeff King
2015-08-10 9:36 ` [PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing Jeff King
2015-08-10 10:34 ` Michael Haggerty
2015-08-10 12:26 ` Jeff King
2015-08-10 9:36 ` [PATCH 12/17] refs.c: avoid repeated git_path calls in rename_tmp_log Jeff King
2015-08-10 9:37 ` [PATCH 13/17] refs.c: avoid git_path assignment in lock_ref_sha1_basic Jeff King
2015-08-10 9:37 ` [PATCH 14/17] refs.c: remove_empty_directories can take a strbuf Jeff King
2015-08-10 9:37 ` [PATCH 15/17] find_hook: keep our own static buffer Jeff King
2015-08-10 9:37 ` [PATCH 16/17] get_repo_path: refactor path-allocation Jeff King
2015-08-10 9:38 ` [PATCH 17/17] memoize common git-path "constant" files Jeff King
2015-08-10 12:05 ` Michael Haggerty
2015-08-10 12:30 ` Jeff King
2015-08-10 12:06 ` [PATCH 0/17] removing questionable uses of git_path Michael Haggerty
2015-08-10 17:31 ` Junio C Hamano
2015-08-10 17:47 ` Jeff King
2015-08-15 9:05 ` Duy Nguyen
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150810092731.GA9027@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=mhagger@alum.mit.edu \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).