Re: [PATCH/POC 1/7] Make git_path() beware of file relocation in $GIT_DIR
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:59:27
Nguyễn Thái Ngọc Duy [off-list ref] writes:
quoted hunk
+static void adjust_git_path(char *buf, int git_dir_len) +{ + /* XXX buffer overflow */ + char *base = buf + git_dir_len; + if (git_graft_env && !strcmp(base, "info/grafts")) + strcpy(buf, get_graft_file()); + else if (git_index_env && !strcmp(base, "index")) + strcpy(buf, get_index_file()); + else if (git_db_env && dir_prefix(base, "objects")) + replace_dir(buf, git_dir_len + 7, get_object_directory()); +} + static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args) { const char *git_dir = get_git_dir(); - size_t len; + size_t len, total_len; len = strlen(git_dir); if (n < len + 1)@@ -60,9 +88,10 @@ static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args) memcpy(buf, git_dir, len); if (len && !is_dir_sep(git_dir[len-1])) buf[len++] = '/'; - len += vsnprintf(buf + len, n - len, fmt, args); - if (len >= n) + total_len = len + vsnprintf(buf + len, n - len, fmt, args); + if (total_len >= n) goto bad; + adjust_git_path(buf, len);
This is a minor tangent, but this part of the patch made me raise my
eyebrow, wondering what Git-specific path mangler is doing in a
function vsnpath that is named as if it is a lot more generic, until
I read the change in context.
The vnspath() is already Git-specific---it is a helper that is used
to create a path inside the $GIT_DIR directory.
We probably should do two things to clear things up:
- Right now, path.c has definitions of functions in this order:
char *mksnpath(char *buf, size_t n, const char *fmt, ...)
static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args)
char *git_snpath(char *buf, size_t n, const char *fmt, ...)
char *git_pathdup(const char *fmt, ...)
char *mkpathdup(const char *fmt, ...)
char *mkpath(const char *fmt, ...)
char *git_path(const char *fmt, ...)
The two functions mkpathdup() and mkpath() are not Git specific
at all. They should be moved up to be grouped together with
mksnpath() that is also not Git-specific.
- Rename the static vsnpath() to further clarify that it is Git
specific.