Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage
- From
Nguyen Thai Ngoc Duy <pclouds@gmail.com>
- Date
- Nov 18, 2011, 03:33 UTC
- Message-ID
- <20111118033349.GA20827@tre>
- In-Reply-To
- <20111116075955.GB13706@elie.hsd1.il.comcast.net>
On Wed, Nov 16, 2011 at 01:59:55AM -0600, Jonathan Nieder wrote:
Show 18 quoted lines
> Ramkumar Ramachandra wrote: > > Junio C Hamano wrote: > > >> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436 > > > > I noticed that sha1_to_hex() also operates like this. The > > resolve_ref() function is really important, but using the same > > technique for these tiny functions is probably an overkill > > I don't follow. Do you mean that not being confusing is overkill, > because the function is small that no one will bother to look up the > right semantics? Wait, that sentence didn't come out the way I > wanted. ;-) > > Jokes aside, here's a rough series to do the git_path -> > git_path_unsafe renaming. While writing it, I noticed a couple of > bugs, hence the two patches before the last one. Patch 2 is the more > interesting one.
Or perhaps we can use per-file buffer rings instead of a global one. This means git_path() can only interfere another one in the same file, making the interaction simpler and hopefully simple enough for reviewers to catch 90% bugs, therefore safe enough to avoid the _unsafe suffix.
Adding static variable declaration in cache.h is ugly, but that could be moved to a separate header file.
diff --git a/cache.h b/cache.h index 2e6ad36..437bc3a 100644 --- a/cache.h +++ b/cache.h @@ -660,9 +660,13 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...) extern char *git_pathdup(const char *fmt, ...) __attribute__((format (printf, 1, 2))); +#define git_path(...) git_path_1(pathname_array[3 & ++pathname_index], __VA_ARGS__) +static char pathname_array[4][PATH_MAX]; +static int pathname_index; + /* Return a statically allocated filename matching the sha1 signature */ extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2))); -extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2))); +extern char *git_path_1(char *pathname, const char *fmt, ...) __attribute__((format (printf, 2, 3))); extern char *git_path_submodule(const char *path, const char *fmt, ...) __attribute__((format (printf, 2, 3))); diff --git a/path.c b/path.c index b6f71d1..3c95db1 100644 --- a/path.c +++ b/path.c @@ -101,10 +101,9 @@ char *mkpath(const char *fmt, ...) return cleanup_path(pathname); } -char *git_path(const char *fmt, ...) +char *git_path_1(char *pathname, const char *fmt, ...) { const char *git_dir = get_git_dir(); - char *pathname = get_pathname(); va_list args; unsigned len;