* Re: [PATCH] Add mksnpath and git_snpath which allow to specify the output buffer
2008-10-28 3:35 ` Junio C Hamano
@ 2008-10-28 12:47 ` Alex Riesen
2008-10-28 17:31 ` Alex Riesen
0 siblings, 1 reply; 10+ messages in thread
From: Alex Riesen @ 2008-10-28 12:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linus Torvalds
[-- Attachment #1: Type: text/plain, Size: 545 bytes --]
2008/10/28 Junio C Hamano <gitster@pobox.com>:
> Alex Riesen <raa.lkml@gmail.com> writes:
>> Maybe I should resend the patches without it, following by patches
>> introducing git_snpath and replacing calls to git_path.
>
> I took the liberty of doing the first half of just that ;-)
>
Thanks. And am sorry... I did that too, and stupidly forgot to send.
I also considered replacing xstrdup(mkpath) with a function which does
just that (patches 8-9). Patches 1 and 2 are unrelated, will send them
separately.
FWIW now, I'm sending the patches.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-Add-mksnpath-which-allows-to-specify-the-output-buff.patch --]
[-- Type: text/x-diff; name=0003-Add-mksnpath-which-allows-to-specify-the-output-buff.patch, Size: 1653 bytes --]
From dcea4c409f4deadbc176b43e8cdbfd5cd9edac3f Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Mon, 27 Oct 2008 10:18:06 +0100
Subject: [PATCH] Add mksnpath which allows to specify the output buffer
To be used as alternative to mkpath where the buffer for the created
path may not be be reused by subsequent calls of the function or will
be copied anyway.
It is actually just a vsnprintf's but additionally calls cleanup_path
on the result.
---
cache.h | 2 ++
path.c | 15 +++++++++++++++
2 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index b0edbf9..9efa602 100644
--- a/cache.h
+++ b/cache.h
@@ -495,6 +495,8 @@ extern int check_repository_format(void);
#define DATA_CHANGED 0x0020
#define TYPE_CHANGED 0x0040
+extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
+ __attribute__((format (printf, 3, 4)));
/* 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)));
diff --git a/path.c b/path.c
index 76e8872..8b64878 100644
--- a/path.c
+++ b/path.c
@@ -32,6 +32,21 @@ static char *cleanup_path(char *path)
return path;
}
+char *mksnpath(char *buf, size_t n, const char *fmt, ...)
+{
+ va_list args;
+ unsigned len;
+
+ va_start(args, fmt);
+ len = vsnprintf(buf, n, fmt, args);
+ va_end(args);
+ if (len >= n) {
+ snprintf(buf, n, bad_path);
+ return buf;
+ }
+ return cleanup_path(buf);
+}
+
char *mkpath(const char *fmt, ...)
{
va_list args;
--
1.6.0.3.549.gb475d
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0004-Fix-mkpath-abuse-in-dwim_ref-sha1_name.c.patch --]
[-- Type: text/x-diff; name=0004-Fix-mkpath-abuse-in-dwim_ref-sha1_name.c.patch, Size: 1438 bytes --]
From 14fcf0fe5cd3baec1706a81876698a27adcda1fa Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 14 Oct 2008 18:23:49 +0200
Subject: [PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c
Otherwise the function sometimes fail to resolve obviously correct refnames,
because the string data pointed to by "ref" argument were reused.
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
sha1_name.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 41b6809..159c2ab 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -245,11 +245,13 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
*ref = NULL;
for (p = ref_rev_parse_rules; *p; p++) {
+ char fullref[PATH_MAX];
unsigned char sha1_from_ref[20];
unsigned char *this_result;
this_result = refs_found ? sha1_from_ref : sha1;
- r = resolve_ref(mkpath(*p, len, str), this_result, 1, NULL);
+ mksnpath(fullref, sizeof(fullref), *p, len, str);
+ r = resolve_ref(fullref, this_result, 1, NULL);
if (r) {
if (!refs_found++)
*ref = xstrdup(r);
@@ -272,7 +274,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
char path[PATH_MAX];
const char *ref, *it;
- strcpy(path, mkpath(*p, len, str));
+ mksnpath(path, sizeof(path), *p, len, str);
ref = resolve_ref(path, hash, 1, NULL);
if (!ref)
continue;
--
1.6.0.3.549.gb475d
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0005-Fix-potentially-dangerous-use-of-mkpath.patch --]
[-- Type: text/x-diff; name=0005-Fix-potentially-dangerous-use-of-mkpath.patch, Size: 2109 bytes --]
From 9f751c2a681aed2089ba30f64a0478ea3e68a81c Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Sun, 19 Oct 2008 20:17:17 +0200
Subject: [PATCH] Fix potentially dangerous use of mkpath
In the changed code a pointer to the buffer returned by mkpath is used
after a function is called which also uses mkpath or git_path. As
both these functions use the same ring of buffers, the data pointed by
the pointer stored in the first function can be overwritten when the
function returns, not to mention the possibility that other code using
the same buffer ring can come in in the future.
Replace mkpath with mksnpath and a local buffer for the resulting
string.
---
builtin-apply.c | 4 ++--
builtin-for-each-ref.c | 6 ++++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index cfd8fce..4c4d1e1 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2841,8 +2841,8 @@ static void create_one_file(char *path, unsigned mode, const char *buf, unsigned
unsigned int nr = getpid();
for (;;) {
- const char *newpath;
- newpath = mkpath("%s~%u", path, nr);
+ char newpath[PATH_MAX];
+ mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr);
if (!try_create_file(newpath, mode, buf, size)) {
if (!rename(newpath, path))
return;
diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index fa6c1ed..e46b7ad 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -620,14 +620,16 @@ static char *get_short_ref(struct refinfo *ref)
for (j = 0; j < i; j++) {
const char *rule = ref_rev_parse_rules[j];
unsigned char short_objectname[20];
+ char refname[PATH_MAX];
/*
* the short name is ambiguous, if it resolves
* (with this previous rule) to a valid ref
* read_ref() returns 0 on success
*/
- if (!read_ref(mkpath(rule, short_name_len, short_name),
- short_objectname))
+ mksnpath(refname, sizeof(refname),
+ rule, short_name_len, short_name);
+ if (!read_ref(refname, short_objectname))
break;
}
--
1.6.0.3.549.gb475d
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0006-Add-git_snpath-a-.git-path-formatting-routine-with.patch --]
[-- Type: text/x-diff; name=0006-Add-git_snpath-a-.git-path-formatting-routine-with.patch, Size: 1881 bytes --]
From cf5fe91d172aca73b79c05aef261fb7040749127 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Mon, 27 Oct 2008 10:22:21 +0100
Subject: [PATCH] Add git_snpath: a .git path formatting routine with output buffer
The function's purpose is to replace git_path where the buffer of
formatted path may not be reused by subsequent calls of the function
or will be copied anyway.
---
cache.h | 2 ++
path.c | 23 +++++++++++++++++++++++
2 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index 9efa602..a9024db 100644
--- a/cache.h
+++ b/cache.h
@@ -497,6 +497,8 @@ extern int check_repository_format(void);
extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
+extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
+ __attribute__((format (printf, 3, 4)));
/* 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)));
diff --git a/path.c b/path.c
index 8b64878..85ab28a 100644
--- a/path.c
+++ b/path.c
@@ -47,6 +47,29 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
return cleanup_path(buf);
}
+char *git_snpath(char *buf, size_t n, const char *fmt, ...)
+{
+ const char *git_dir = get_git_dir();
+ va_list args;
+ size_t len;
+
+ len = strlen(git_dir);
+ if (n < len + 1)
+ goto bad;
+ memcpy(buf, git_dir, len);
+ if (len && !is_dir_sep(git_dir[len-1]))
+ buf[len++] = '/';
+ va_start(args, fmt);
+ len += vsnprintf(buf + len, n - len, fmt, args);
+ va_end(args);
+ if (len >= n)
+ goto bad;
+ return cleanup_path(buf);
+bad:
+ snprintf(buf, n, bad_path);
+ return buf;
+}
+
char *mkpath(const char *fmt, ...)
{
va_list args;
--
1.6.0.3.549.gb475d
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0007-Fix-potentially-dangerous-use-of-git_path-in-ref.c.patch --]
[-- Type: text/x-diff; name=0007-Fix-potentially-dangerous-use-of-git_path-in-ref.c.patch, Size: 1506 bytes --]
From 109d1b74e2b093bd63419fb1d0f720fa54a476f3 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Mon, 27 Oct 2008 11:11:40 +0100
Subject: [PATCH] Fix potentially dangerous use of git_path in ref.c
---
refs.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 0a126fa..68189ba 100644
--- a/refs.c
+++ b/refs.c
@@ -413,7 +413,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
*flag = 0;
for (;;) {
- const char *path = git_path("%s", ref);
+ char path[PATH_MAX];
struct stat st;
char *buf;
int fd;
@@ -421,6 +421,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
if (--depth < 0)
return NULL;
+ git_snpath(path, sizeof(path), "%s", ref);
/* Special case: non-existing file. */
if (lstat(path, &st) < 0) {
struct ref_list *list = get_packed_refs();
@@ -1130,13 +1131,14 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
int logfd, written, oflags = O_APPEND | O_WRONLY;
unsigned maxlen, len;
int msglen;
- char *log_file, *logrec;
+ char log_file[PATH_MAX];
+ char *logrec;
const char *committer;
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
- log_file = git_path("logs/%s", ref_name);
+ git_snpath(log_file, sizeof(log_file), "logs/%s", ref_name);
if (log_all_ref_updates &&
(!prefixcmp(ref_name, "refs/heads/") ||
--
1.6.0.3.549.gb475d
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #7: 0008-git_pathdup-returns-xstrdup-ed-copy-of-the-formatte.patch --]
[-- Type: text/x-diff; name=0008-git_pathdup-returns-xstrdup-ed-copy-of-the-formatte.patch, Size: 2252 bytes --]
From bb22051c0925039b5161bc0f49e3098f9965a069 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Mon, 27 Oct 2008 11:17:51 +0100
Subject: [PATCH] git_pathdup: returns xstrdup-ed copy of the formatted path
---
cache.h | 2 ++
path.c | 24 ++++++++++++++++++++----
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/cache.h b/cache.h
index a9024db..981b4e6 100644
--- a/cache.h
+++ b/cache.h
@@ -499,6 +499,8 @@ extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
+extern char *git_pathdup(const char *fmt, ...)
+ __attribute__((format (printf, 1, 2)));
/* 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)));
diff --git a/path.c b/path.c
index 85ab28a..092ce57 100644
--- a/path.c
+++ b/path.c
@@ -47,10 +47,9 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
return cleanup_path(buf);
}
-char *git_snpath(char *buf, size_t n, const char *fmt, ...)
+static char *git_vsnpath(char *buf, size_t n, const char *fmt, va_list args)
{
const char *git_dir = get_git_dir();
- va_list args;
size_t len;
len = strlen(git_dir);
@@ -59,9 +58,7 @@ char *git_snpath(char *buf, size_t n, const char *fmt, ...)
memcpy(buf, git_dir, len);
if (len && !is_dir_sep(git_dir[len-1]))
buf[len++] = '/';
- va_start(args, fmt);
len += vsnprintf(buf + len, n - len, fmt, args);
- va_end(args);
if (len >= n)
goto bad;
return cleanup_path(buf);
@@ -70,6 +67,25 @@ bad:
return buf;
}
+char *git_snpath(char *buf, size_t n, const char *fmt, ...)
+{
+ va_list args;
+ va_start(args, fmt);
+ (void)git_vsnpath(buf, n, fmt, args);
+ va_end(args);
+ return buf;
+}
+
+char *git_pathdup(const char *fmt, ...)
+{
+ char path[PATH_MAX];
+ va_list args;
+ va_start(args, fmt);
+ (void)git_vsnpath(path, sizeof(path), fmt, args);
+ va_end(args);
+ return xstrdup(path);
+}
+
char *mkpath(const char *fmt, ...)
{
va_list args;
--
1.6.0.3.549.gb475d
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #8: 0009-Use-git_pathdup-instead-of-xstrdup-git_path.patch --]
[-- Type: text/x-diff; name=0009-Use-git_pathdup-instead-of-xstrdup-git_path.patch, Size: 5483 bytes --]
From 3e142004e382ea0e05d030a3be6a8fc0e896f5d1 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Mon, 27 Oct 2008 11:22:09 +0100
Subject: [PATCH] Use git_pathdup instead of xstrdup(git_path(...))
---
builtin-config.c | 2 +-
builtin-reflog.c | 4 ++--
builtin-revert.c | 2 +-
builtin-tag.c | 2 +-
config.c | 6 +++---
environment.c | 2 +-
refs.c | 2 +-
rerere.c | 2 +-
server-info.c | 2 +-
9 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/builtin-config.c b/builtin-config.c
index 91fdc49..f710162 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -84,7 +84,7 @@ static int get_value(const char* key_, const char* regex_)
local = config_exclusive_filename;
if (!local) {
const char *home = getenv("HOME");
- local = repo_config = xstrdup(git_path("config"));
+ local = repo_config = git_pathdup("config");
if (git_config_global() && home)
global = xstrdup(mkpath("%s/.gitconfig", home));
if (git_config_system())
diff --git a/builtin-reflog.c b/builtin-reflog.c
index 6b3667e..d95f515 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -277,11 +277,11 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
lock = lock_any_ref_for_update(ref, sha1, 0);
if (!lock)
return error("cannot lock ref '%s'", ref);
- log_file = xstrdup(git_path("logs/%s", ref));
+ log_file = git_pathdup("logs/%s", ref);
if (!file_exists(log_file))
goto finish;
if (!cmd->dry_run) {
- newlog_path = xstrdup(git_path("logs/%s.lock", ref));
+ newlog_path = git_pathdup("logs/%s.lock", ref);
cb.newlog = fopen(newlog_path, "w");
}
diff --git a/builtin-revert.c b/builtin-revert.c
index 7483a7a..4038b41 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -251,7 +251,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
int i, index_fd, clean;
char *oneline, *reencoded_message = NULL;
const char *message, *encoding;
- char *defmsg = xstrdup(git_path("MERGE_MSG"));
+ char *defmsg = git_pathdup("MERGE_MSG");
struct merge_options o;
struct tree *result, *next_tree, *base_tree, *head_tree;
static struct lock_file index_lock;
diff --git a/builtin-tag.c b/builtin-tag.c
index b13fa34..efd7723 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -283,7 +283,7 @@ static void create_tag(const unsigned char *object, const char *tag,
int fd;
/* write the template message before editing: */
- path = xstrdup(git_path("TAG_EDITMSG"));
+ path = git_pathdup("TAG_EDITMSG");
fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
if (fd < 0)
die("could not create file '%s': %s",
diff --git a/config.c b/config.c
index b8d289d..67cc1dc 100644
--- a/config.c
+++ b/config.c
@@ -649,7 +649,7 @@ int git_config(config_fn_t fn, void *data)
free(user_config);
}
- repo_config = xstrdup(git_path("config"));
+ repo_config = git_pathdup("config");
ret += git_config_from_file(fn, repo_config, data);
free(repo_config);
return ret;
@@ -889,7 +889,7 @@ int git_config_set_multivar(const char* key, const char* value,
if (config_exclusive_filename)
config_filename = xstrdup(config_exclusive_filename);
else
- config_filename = xstrdup(git_path("config"));
+ config_filename = git_pathdup("config");
/*
* Since "key" actually contains the section name and the real
@@ -1149,7 +1149,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)
if (config_exclusive_filename)
config_filename = xstrdup(config_exclusive_filename);
else
- config_filename = xstrdup(git_path("config"));
+ config_filename = git_pathdup("config");
out_fd = hold_lock_file_for_update(lock, config_filename, 0);
if (out_fd < 0) {
ret = error("could not lock config file %s", config_filename);
diff --git a/environment.c b/environment.c
index 0693cd9..bf93a59 100644
--- a/environment.c
+++ b/environment.c
@@ -71,7 +71,7 @@ static void setup_git_env(void)
}
git_graft_file = getenv(GRAFT_ENVIRONMENT);
if (!git_graft_file)
- git_graft_file = xstrdup(git_path("info/grafts"));
+ git_graft_file = git_pathdup("info/grafts");
}
int is_bare_repository(void)
diff --git a/refs.c b/refs.c
index 68189ba..01e4df4 100644
--- a/refs.c
+++ b/refs.c
@@ -1267,7 +1267,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
const char *lockpath;
char ref[1000];
int fd, len, written;
- char *git_HEAD = xstrdup(git_path("%s", ref_target));
+ char *git_HEAD = git_pathdup("%s", ref_target);
unsigned char old_sha1[20], new_sha1[20];
if (logmsg && read_ref(ref_target, old_sha1))
diff --git a/rerere.c b/rerere.c
index 8e5532b..02931a1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -351,7 +351,7 @@ int setup_rerere(struct string_list *merge_rr)
if (!is_rerere_enabled())
return -1;
- merge_rr_path = xstrdup(git_path("MERGE_RR"));
+ merge_rr_path = git_pathdup("MERGE_RR");
fd = hold_lock_file_for_update(&write_lock, merge_rr_path,
LOCK_DIE_ON_ERROR);
read_rr(merge_rr);
diff --git a/server-info.c b/server-info.c
index c1c073b..66b0d9d 100644
--- a/server-info.c
+++ b/server-info.c
@@ -25,7 +25,7 @@ static int add_info_ref(const char *path, const unsigned char *sha1, int flag, v
static int update_info_refs(int force)
{
- char *path0 = xstrdup(git_path("info/refs"));
+ char *path0 = git_pathdup("info/refs");
int len = strlen(path0);
char *path1 = xmalloc(len + 2);
--
1.6.0.3.549.gb475d
^ permalink raw reply related [flat|nested] 10+ messages in thread