{"thread":{"id":"16053","subject":"[PATCH] Add mksnpath and git_snpath which allow to specify the output buffer","startedAt":"2008-10-26T21:59:13Z","lastAt":"2008-10-28T17:31:32Z","messageCount":10,"participants":["Alex Riesen","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"94001","messageId":"20081026215913.GA18594@blimp.localdomain","threadId":"16053","inReplyTo":null,"subject":"[PATCH] Add mksnpath and git_snpath which allow to specify the output buffer","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-26T21:59:13Z","receivedAt":"2008-10-26T21:59:13Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Both are actually just vsnprintf's but additionally call cleanup_path\non the result. To be used as alternatives to mkpath and git_path where\nthe buffer for the created path may not be reused by subsequent calls\nof the same formatting function.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n cache.h |    4 ++++\n path.c  |   38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b0edbf9..a9024db 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -495,6 +495,10 @@ extern int check_repository_format(void);\n #define DATA_CHANGED    0x0020\n #define TYPE_CHANGED    0x0040\n \n+extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n+\t__attribute__((format (printf, 3, 4)));\n+extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n+\t__attribute__((format (printf, 3, 4)));\n /* Return a statically allocated filename matching the sha1 signature */\n extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/path.c b/path.c\nindex 76e8872..85ab28a 100644\n--- a/path.c\n+++ b/path.c\n@@ -32,6 +32,44 @@ static char *cleanup_path(char *path)\n \treturn path;\n }\n \n+char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n+{\n+\tva_list args;\n+\tunsigned len;\n+\n+\tva_start(args, fmt);\n+\tlen = vsnprintf(buf, n, fmt, args);\n+\tva_end(args);\n+\tif (len >= n) {\n+\t\tsnprintf(buf, n, bad_path);\n+\t\treturn buf;\n+\t}\n+\treturn cleanup_path(buf);\n+}\n+\n+char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n+{\n+\tconst char *git_dir = get_git_dir();\n+\tva_list args;\n+\tsize_t len;\n+\n+\tlen = strlen(git_dir);\n+\tif (n < len + 1)\n+\t\tgoto bad;\n+\tmemcpy(buf, git_dir, len);\n+\tif (len && !is_dir_sep(git_dir[len-1]))\n+\t\tbuf[len++] = '/';\n+\tva_start(args, fmt);\n+\tlen += vsnprintf(buf + len, n - len, fmt, args);\n+\tva_end(args);\n+\tif (len >= n)\n+\t\tgoto bad;\n+\treturn cleanup_path(buf);\n+bad:\n+\tsnprintf(buf, n, bad_path);\n+\treturn buf;\n+}\n+\n char *mkpath(const char *fmt, ...)\n {\n \tva_list args;\n-- \n1.6.0.3.540.g3f8b\n"},{"id":"94003","messageId":"20081026220724.GB18594@blimp.localdomain","threadId":"16053","inReplyTo":"20081026215913.GA18594@blimp.localdomain","subject":"[PATCH] Fix mkpath abuse in dwim_ref and dwim_log of sha1_name.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-26T22:07:24Z","receivedAt":"2008-10-26T22:07:24Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Otherwise the function sometimes fail to resolve obviously correct\nrefnames, because the string data pointed to by \"str\" argument were\nreused.\n\nThe change in dwim_log does not fix anything, just optimizes away\nstrcpy code as the path can be created directly in the available\nbuffer.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nWas noticed in cygwin port, which somehow (supposedly by excessive\ncalling of git_config from lstat stub setup) managed to reuse the\njust returned buffer.\n\n sha1_name.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 41b6809..159c2ab 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -245,11 +245,13 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \n \t*ref = NULL;\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n+\t\tchar fullref[PATH_MAX];\n \t\tunsigned char sha1_from_ref[20];\n \t\tunsigned char *this_result;\n \n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n-\t\tr = resolve_ref(mkpath(*p, len, str), this_result, 1, NULL);\n+\t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n+\t\tr = resolve_ref(fullref, this_result, 1, NULL);\n \t\tif (r) {\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n@@ -272,7 +274,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tchar path[PATH_MAX];\n \t\tconst char *ref, *it;\n \n-\t\tstrcpy(path, mkpath(*p, len, str));\n+\t\tmksnpath(path, sizeof(path), *p, len, str);\n \t\tref = resolve_ref(path, hash, 1, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n-- \n1.6.0.3.540.g3f8b\n"},{"id":"94004","messageId":"20081026220852.GC18594@blimp.localdomain","threadId":"16053","inReplyTo":"20081026215913.GA18594@blimp.localdomain","subject":"[PATCH] Fix potentially dangerous uses of mkpath and git_path","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-26T22:08:52Z","receivedAt":"2008-10-26T22:08:52Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Replace them  with mksnpath/git_snpath and a local buffer\nfor the resulting string.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n builtin-apply.c        |    4 ++--\n builtin-for-each-ref.c |    6 ++++--\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex cfd8fce..4c4d1e1 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2841,8 +2841,8 @@ static void create_one_file(char *path, unsigned mode, const char *buf, unsigned\n \t\tunsigned int nr = getpid();\n \n \t\tfor (;;) {\n-\t\t\tconst char *newpath;\n-\t\t\tnewpath = mkpath(\"%s~%u\", path, nr);\n+\t\t\tchar newpath[PATH_MAX];\n+\t\t\tmksnpath(newpath, sizeof(newpath), \"%s~%u\", path, nr);\n \t\t\tif (!try_create_file(newpath, mode, buf, size)) {\n \t\t\t\tif (!rename(newpath, path))\n \t\t\t\t\treturn;\ndiff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c\nindex fa6c1ed..e46b7ad 100644\n--- a/builtin-for-each-ref.c\n+++ b/builtin-for-each-ref.c\n@@ -620,14 +620,16 @@ static char *get_short_ref(struct refinfo *ref)\n \t\tfor (j = 0; j < i; j++) {\n \t\t\tconst char *rule = ref_rev_parse_rules[j];\n \t\t\tunsigned char short_objectname[20];\n+\t\t\tchar refname[PATH_MAX];\n \n \t\t\t/*\n \t\t\t * the short name is ambiguous, if it resolves\n \t\t\t * (with this previous rule) to a valid ref\n \t\t\t * read_ref() returns 0 on success\n \t\t\t */\n-\t\t\tif (!read_ref(mkpath(rule, short_name_len, short_name),\n-\t\t\t\t      short_objectname))\n+\t\t\tmksnpath(refname, sizeof(refname),\n+\t\t\t\t rule, short_name_len, short_name);\n+\t\t\tif (!read_ref(refname, short_objectname))\n \t\t\t\tbreak;\n \t\t}\n \n-- \n1.6.0.3.540.g3f8b\n"},{"id":"94018","messageId":"7v8wsak4mr.fsf@gitster.siamese.dyndns.org","threadId":"16053","inReplyTo":"20081026215913.GA18594@blimp.localdomain","subject":"Re: [PATCH] Add mksnpath and git_snpath which allow to specify the output buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-27T05:07:24Z","receivedAt":"2008-10-27T05:07:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Where is git_snpath() used?\n"},{"id":"94025","messageId":"20081027064537.GA4751@blimp.localdomain","threadId":"16053","inReplyTo":"7v8wsak4mr.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add mksnpath and git_snpath which allow to specify the output buffer","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-27T06:45:37Z","receivedAt":"2008-10-27T06:45:37Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Mon, Oct 27, 2008 06:07:24 +0100:\n> Where is git_snpath() used?\n\nNowhere yet, but it should replace git_path in every call where the\nresult is not used immediately. Which, as the story with cygwin\nporting shows, can be sometimes not quite trivial (who could suspect\nlstat(2) will have application local side effects?).\n\nMaybe I should resend the patches without it, following by patches\nintroducing git_snpath and replacing calls to git_path.\n\nMaybe Linus should be sued for introducing the function in the first\nplace.\n"},{"id":"94026","messageId":"490568F5.9060206@viscovery.net","threadId":"16053","inReplyTo":"20081026220852.GC18594@blimp.localdomain","subject":"Re: [PATCH] Fix potentially dangerous uses of mkpath and git_path","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-10-27T07:08:37Z","receivedAt":"2008-10-27T07:08:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Alex Riesen schrieb:\n> Replace them  with mksnpath/git_snpath and a local buffer\n> for the resulting string.\n\nYou should describe your definition of \"potentially dangerous\" to save the\nreader some time.\n\n-- Hannes\n"},{"id":"94032","messageId":"81b0412b0810270130w6cbfc17dy444f962666cc6c9a@mail.gmail.com","threadId":"16053","inReplyTo":"490568F5.9060206@viscovery.net","subject":"Re: [PATCH] Fix potentially dangerous uses of mkpath and git_path","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-27T08:30:20Z","receivedAt":"2008-10-27T08:30:20Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/27 Johannes Sixt <j.sixt@viscovery.net>:\n> Alex Riesen schrieb:\n>> Replace them  with mksnpath/git_snpath and a local buffer\n>> for the resulting string.\n>\n> You should describe your definition of \"potentially dangerous\" to save the\n> reader some time.\n>\n\nYeah. Thought it was obvious. Will do.\n"},{"id":"94090","messageId":"7vd4hlgzo7.fsf@gitster.siamese.dyndns.org","threadId":"16053","inReplyTo":"20081027064537.GA4751@blimp.localdomain","subject":"Re: [PATCH] Add mksnpath and git_snpath which allow to specify the output buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-28T03:35:04Z","receivedAt":"2008-10-28T03:35:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> Junio C Hamano, Mon, Oct 27, 2008 06:07:24 +0100:\n>> Where is git_snpath() used?\n>\n> Nowhere yet, but it should replace git_path in every call where the\n> result is not used immediately. Which, as the story with cygwin\n> porting shows, can be sometimes not quite trivial (who could suspect\n> lstat(2) will have application local side effects?).\n>\n> Maybe I should resend the patches without it, following by patches\n> introducing git_snpath and replacing calls to git_path.\n\nI took the liberty of doing the first half of just that ;-)\n\nI also think this series of fix is maint-worthy, and splitted the last one\ninto two so that maint and master can be fixed independently.\n"},{"id":"94103","messageId":"81b0412b0810280547nf5e6834t4049e92d374926b6@mail.gmail.com","threadId":"16053","inReplyTo":"7vd4hlgzo7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add mksnpath and git_snpath which allow to specify the output buffer","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-28T12:47:21Z","receivedAt":"2008-10-28T12:47:21Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/28 Junio C Hamano <gitster@pobox.com>:\n> Alex Riesen <raa.lkml@gmail.com> writes:\n>> Maybe I should resend the patches without it, following by patches\n>> introducing git_snpath and replacing calls to git_path.\n>\n> I took the liberty of doing the first half of just that ;-)\n>\n\nThanks. And am sorry... I did that too, and stupidly forgot to send.\nI also considered replacing xstrdup(mkpath) with a function which does\njust that (patches 8-9). Patches 1 and 2 are unrelated, will send them\nseparately.\n\nFWIW now, I'm sending the patches.\n\n\nFrom dcea4c409f4deadbc176b43e8cdbfd5cd9edac3f Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen <raa.lkml@gmail.com>\nDate: Mon, 27 Oct 2008 10:18:06 +0100\nSubject: [PATCH] Add mksnpath which allows to specify the output buffer\n\nTo be used as alternative to mkpath where the buffer for the created\npath may not be be reused by subsequent calls of the function or will\nbe copied anyway.\n\nIt is actually just a vsnprintf's but additionally calls cleanup_path\non the result.\n---\n cache.h |    2 ++\n path.c  |   15 +++++++++++++++\n 2 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b0edbf9..9efa602 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -495,6 +495,8 @@ extern int check_repository_format(void);\n #define DATA_CHANGED    0x0020\n #define TYPE_CHANGED    0x0040\n \n+extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n+\t__attribute__((format (printf, 3, 4)));\n /* Return a statically allocated filename matching the sha1 signature */\n extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/path.c b/path.c\nindex 76e8872..8b64878 100644\n--- a/path.c\n+++ b/path.c\n@@ -32,6 +32,21 @@ static char *cleanup_path(char *path)\n \treturn path;\n }\n \n+char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n+{\n+\tva_list args;\n+\tunsigned len;\n+\n+\tva_start(args, fmt);\n+\tlen = vsnprintf(buf, n, fmt, args);\n+\tva_end(args);\n+\tif (len >= n) {\n+\t\tsnprintf(buf, n, bad_path);\n+\t\treturn buf;\n+\t}\n+\treturn cleanup_path(buf);\n+}\n+\n char *mkpath(const char *fmt, ...)\n {\n \tva_list args;\n-- \n1.6.0.3.549.gb475d\n\n\n\nFrom 14fcf0fe5cd3baec1706a81876698a27adcda1fa Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen <raa.lkml@gmail.com>\nDate: Tue, 14 Oct 2008 18:23:49 +0200\nSubject: [PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c\n\nOtherwise the function sometimes fail to resolve obviously correct refnames,\nbecause the string data pointed to by \"ref\" argument were reused.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n sha1_name.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 41b6809..159c2ab 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -245,11 +245,13 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \n \t*ref = NULL;\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n+\t\tchar fullref[PATH_MAX];\n \t\tunsigned char sha1_from_ref[20];\n \t\tunsigned char *this_result;\n \n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n-\t\tr = resolve_ref(mkpath(*p, len, str), this_result, 1, NULL);\n+\t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n+\t\tr = resolve_ref(fullref, this_result, 1, NULL);\n \t\tif (r) {\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n@@ -272,7 +274,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tchar path[PATH_MAX];\n \t\tconst char *ref, *it;\n \n-\t\tstrcpy(path, mkpath(*p, len, str));\n+\t\tmksnpath(path, sizeof(path), *p, len, str);\n \t\tref = resolve_ref(path, hash, 1, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n-- \n1.6.0.3.549.gb475d\n\n\n\nFrom 9f751c2a681aed2089ba30f64a0478ea3e68a81c Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen <raa.lkml@gmail.com>\nDate: Sun, 19 Oct 2008 20:17:17 +0200\nSubject: [PATCH] Fix potentially dangerous use of mkpath\n\nIn the changed code a pointer to the buffer returned by mkpath is used\nafter a function is called which also uses mkpath or git_path.  As\nboth these functions use the same ring of buffers, the data pointed by\nthe pointer stored in the first function can be overwritten when the\nfunction returns, not to mention the possibility that other code using\nthe same buffer ring can come in in the future.\n\nReplace mkpath with mksnpath and a local buffer for the resulting\nstring.\n---\n builtin-apply.c        |    4 ++--\n builtin-for-each-ref.c |    6 ++++--\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex cfd8fce..4c4d1e1 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2841,8 +2841,8 @@ static void create_one_file(char *path, unsigned mode, const char *buf, unsigned\n \t\tunsigned int nr = getpid();\n \n \t\tfor (;;) {\n-\t\t\tconst char *newpath;\n-\t\t\tnewpath = mkpath(\"%s~%u\", path, nr);\n+\t\t\tchar newpath[PATH_MAX];\n+\t\t\tmksnpath(newpath, sizeof(newpath), \"%s~%u\", path, nr);\n \t\t\tif (!try_create_file(newpath, mode, buf, size)) {\n \t\t\t\tif (!rename(newpath, path))\n \t\t\t\t\treturn;\ndiff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c\nindex fa6c1ed..e46b7ad 100644\n--- a/builtin-for-each-ref.c\n+++ b/builtin-for-each-ref.c\n@@ -620,14 +620,16 @@ static char *get_short_ref(struct refinfo *ref)\n \t\tfor (j = 0; j < i; j++) {\n \t\t\tconst char *rule = ref_rev_parse_rules[j];\n \t\t\tunsigned char short_objectname[20];\n+\t\t\tchar refname[PATH_MAX];\n \n \t\t\t/*\n \t\t\t * the short name is ambiguous, if it resolves\n \t\t\t * (with this previous rule) to a valid ref\n \t\t\t * read_ref() returns 0 on success\n \t\t\t */\n-\t\t\tif (!read_ref(mkpath(rule, short_name_len, short_name),\n-\t\t\t\t      short_objectname))\n+\t\t\tmksnpath(refname, sizeof(refname),\n+\t\t\t\t rule, short_name_len, short_name);\n+\t\t\tif (!read_ref(refname, short_objectname))\n \t\t\t\tbreak;\n \t\t}\n \n-- \n1.6.0.3.549.gb475d\n\n\n\nFrom cf5fe91d172aca73b79c05aef261fb7040749127 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen <raa.lkml@gmail.com>\nDate: Mon, 27 Oct 2008 10:22:21 +0100\nSubject: [PATCH] Add git_snpath: a .git path formatting routine with output buffer\n\nThe function's purpose is to replace git_path where the buffer of\nformatted path may not be reused by subsequent calls of the function\nor will be copied anyway.\n---\n cache.h |    2 ++\n path.c  |   23 +++++++++++++++++++++++\n 2 files changed, 25 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 9efa602..a9024db 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -497,6 +497,8 @@ extern int check_repository_format(void);\n \n extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n+extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n+\t__attribute__((format (printf, 3, 4)));\n /* Return a statically allocated filename matching the sha1 signature */\n extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/path.c b/path.c\nindex 8b64878..85ab28a 100644\n--- a/path.c\n+++ b/path.c\n@@ -47,6 +47,29 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \treturn cleanup_path(buf);\n }\n \n+char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n+{\n+\tconst char *git_dir = get_git_dir();\n+\tva_list args;\n+\tsize_t len;\n+\n+\tlen = strlen(git_dir);\n+\tif (n < len + 1)\n+\t\tgoto bad;\n+\tmemcpy(buf, git_dir, len);\n+\tif (len && !is_dir_sep(git_dir[len-1]))\n+\t\tbuf[len++] = '/';\n+\tva_start(args, fmt);\n+\tlen += vsnprintf(buf + len, n - len, fmt, args);\n+\tva_end(args);\n+\tif (len >= n)\n+\t\tgoto bad;\n+\treturn cleanup_path(buf);\n+bad:\n+\tsnprintf(buf, n, bad_path);\n+\treturn buf;\n+}\n+\n char *mkpath(const char *fmt, ...)\n {\n \tva_list args;\n-- \n1.6.0.3.549.gb475d\n\n\n\nFrom 109d1b74e2b093bd63419fb1d0f720fa54a476f3 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen <raa.lkml@gmail.com>\nDate: Mon, 27 Oct 2008 11:11:40 +0100\nSubject: [PATCH] Fix potentially dangerous use of git_path in ref.c\n\n---\n refs.c |    8 +++++---\n 1 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0a126fa..68189ba 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -413,7 +413,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\t*flag = 0;\n \n \tfor (;;) {\n-\t\tconst char *path = git_path(\"%s\", ref);\n+\t\tchar path[PATH_MAX];\n \t\tstruct stat st;\n \t\tchar *buf;\n \t\tint fd;\n@@ -421,6 +421,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\tif (--depth < 0)\n \t\t\treturn NULL;\n \n+\t\tgit_snpath(path, sizeof(path), \"%s\", ref);\n \t\t/* Special case: non-existing file. */\n \t\tif (lstat(path, &st) < 0) {\n \t\t\tstruct ref_list *list = get_packed_refs();\n@@ -1130,13 +1131,14 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n \tint logfd, written, oflags = O_APPEND | O_WRONLY;\n \tunsigned maxlen, len;\n \tint msglen;\n-\tchar *log_file, *logrec;\n+\tchar log_file[PATH_MAX];\n+\tchar *logrec;\n \tconst char *committer;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n \n-\tlog_file = git_path(\"logs/%s\", ref_name);\n+\tgit_snpath(log_file, sizeof(log_file), \"logs/%s\", ref_name);\n \n \tif (log_all_ref_updates &&\n \t    (!prefixcmp(ref_name, \"refs/heads/\") ||\n-- \n1.6.0.3.549.gb475d\n\n\n\nFrom bb22051c0925039b5161bc0f49e3098f9965a069 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen <raa.lkml@gmail.com>\nDate: Mon, 27 Oct 2008 11:17:51 +0100\nSubject: [PATCH] git_pathdup: returns xstrdup-ed copy of the formatted path\n\n---\n cache.h |    2 ++\n path.c  |   24 ++++++++++++++++++++----\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a9024db..981b4e6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -499,6 +499,8 @@ extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n+extern char *git_pathdup(const char *fmt, ...)\n+\t__attribute__((format (printf, 1, 2)));\n /* Return a statically allocated filename matching the sha1 signature */\n extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/path.c b/path.c\nindex 85ab28a..092ce57 100644\n--- a/path.c\n+++ b/path.c\n@@ -47,10 +47,9 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \treturn cleanup_path(buf);\n }\n \n-char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n+static char *git_vsnpath(char *buf, size_t n, const char *fmt, va_list args)\n {\n \tconst char *git_dir = get_git_dir();\n-\tva_list args;\n \tsize_t len;\n \n \tlen = strlen(git_dir);\n@@ -59,9 +58,7 @@ char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n \tmemcpy(buf, git_dir, len);\n \tif (len && !is_dir_sep(git_dir[len-1]))\n \t\tbuf[len++] = '/';\n-\tva_start(args, fmt);\n \tlen += vsnprintf(buf + len, n - len, fmt, args);\n-\tva_end(args);\n \tif (len >= n)\n \t\tgoto bad;\n \treturn cleanup_path(buf);\n@@ -70,6 +67,25 @@ bad:\n \treturn buf;\n }\n \n+char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n+{\n+\tva_list args;\n+\tva_start(args, fmt);\n+\t(void)git_vsnpath(buf, n, fmt, args);\n+\tva_end(args);\n+\treturn buf;\n+}\n+\n+char *git_pathdup(const char *fmt, ...)\n+{\n+\tchar path[PATH_MAX];\n+\tva_list args;\n+\tva_start(args, fmt);\n+\t(void)git_vsnpath(path, sizeof(path), fmt, args);\n+\tva_end(args);\n+\treturn xstrdup(path);\n+}\n+\n char *mkpath(const char *fmt, ...)\n {\n \tva_list args;\n-- \n1.6.0.3.549.gb475d\n\n\n\nFrom 3e142004e382ea0e05d030a3be6a8fc0e896f5d1 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen <raa.lkml@gmail.com>\nDate: Mon, 27 Oct 2008 11:22:09 +0100\nSubject: [PATCH] Use git_pathdup instead of xstrdup(git_path(...))\n\n---\n builtin-config.c |    2 +-\n builtin-reflog.c |    4 ++--\n builtin-revert.c |    2 +-\n builtin-tag.c    |    2 +-\n config.c         |    6 +++---\n environment.c    |    2 +-\n refs.c           |    2 +-\n rerere.c         |    2 +-\n server-info.c    |    2 +-\n 9 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin-config.c b/builtin-config.c\nindex 91fdc49..f710162 100644\n--- a/builtin-config.c\n+++ b/builtin-config.c\n@@ -84,7 +84,7 @@ static int get_value(const char* key_, const char* regex_)\n \tlocal = config_exclusive_filename;\n \tif (!local) {\n \t\tconst char *home = getenv(\"HOME\");\n-\t\tlocal = repo_config = xstrdup(git_path(\"config\"));\n+\t\tlocal = repo_config = git_pathdup(\"config\");\n \t\tif (git_config_global() && home)\n \t\t\tglobal = xstrdup(mkpath(\"%s/.gitconfig\", home));\n \t\tif (git_config_system())\ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex 6b3667e..d95f515 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -277,11 +277,11 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \tlock = lock_any_ref_for_update(ref, sha1, 0);\n \tif (!lock)\n \t\treturn error(\"cannot lock ref '%s'\", ref);\n-\tlog_file = xstrdup(git_path(\"logs/%s\", ref));\n+\tlog_file = git_pathdup(\"logs/%s\", ref);\n \tif (!file_exists(log_file))\n \t\tgoto finish;\n \tif (!cmd->dry_run) {\n-\t\tnewlog_path = xstrdup(git_path(\"logs/%s.lock\", ref));\n+\t\tnewlog_path = git_pathdup(\"logs/%s.lock\", ref);\n \t\tcb.newlog = fopen(newlog_path, \"w\");\n \t}\n \ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex 7483a7a..4038b41 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -251,7 +251,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \tint i, index_fd, clean;\n \tchar *oneline, *reencoded_message = NULL;\n \tconst char *message, *encoding;\n-\tchar *defmsg = xstrdup(git_path(\"MERGE_MSG\"));\n+\tchar *defmsg = git_pathdup(\"MERGE_MSG\");\n \tstruct merge_options o;\n \tstruct tree *result, *next_tree, *base_tree, *head_tree;\n \tstatic struct lock_file index_lock;\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex b13fa34..efd7723 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -283,7 +283,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\tint fd;\n \n \t\t/* write the template message before editing: */\n-\t\tpath = xstrdup(git_path(\"TAG_EDITMSG\"));\n+\t\tpath = git_pathdup(\"TAG_EDITMSG\");\n \t\tfd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n \t\tif (fd < 0)\n \t\t\tdie(\"could not create file '%s': %s\",\ndiff --git a/config.c b/config.c\nindex b8d289d..67cc1dc 100644\n--- a/config.c\n+++ b/config.c\n@@ -649,7 +649,7 @@ int git_config(config_fn_t fn, void *data)\n \t\tfree(user_config);\n \t}\n \n-\trepo_config = xstrdup(git_path(\"config\"));\n+\trepo_config = git_pathdup(\"config\");\n \tret += git_config_from_file(fn, repo_config, data);\n \tfree(repo_config);\n \treturn ret;\n@@ -889,7 +889,7 @@ int git_config_set_multivar(const char* key, const char* value,\n \tif (config_exclusive_filename)\n \t\tconfig_filename = xstrdup(config_exclusive_filename);\n \telse\n-\t\tconfig_filename = xstrdup(git_path(\"config\"));\n+\t\tconfig_filename = git_pathdup(\"config\");\n \n \t/*\n \t * Since \"key\" actually contains the section name and the real\n@@ -1149,7 +1149,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)\n \tif (config_exclusive_filename)\n \t\tconfig_filename = xstrdup(config_exclusive_filename);\n \telse\n-\t\tconfig_filename = xstrdup(git_path(\"config\"));\n+\t\tconfig_filename = git_pathdup(\"config\");\n \tout_fd = hold_lock_file_for_update(lock, config_filename, 0);\n \tif (out_fd < 0) {\n \t\tret = error(\"could not lock config file %s\", config_filename);\ndiff --git a/environment.c b/environment.c\nindex 0693cd9..bf93a59 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -71,7 +71,7 @@ static void setup_git_env(void)\n \t}\n \tgit_graft_file = getenv(GRAFT_ENVIRONMENT);\n \tif (!git_graft_file)\n-\t\tgit_graft_file = xstrdup(git_path(\"info/grafts\"));\n+\t\tgit_graft_file = git_pathdup(\"info/grafts\");\n }\n \n int is_bare_repository(void)\ndiff --git a/refs.c b/refs.c\nindex 68189ba..01e4df4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1267,7 +1267,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n \tconst char *lockpath;\n \tchar ref[1000];\n \tint fd, len, written;\n-\tchar *git_HEAD = xstrdup(git_path(\"%s\", ref_target));\n+\tchar *git_HEAD = git_pathdup(\"%s\", ref_target);\n \tunsigned char old_sha1[20], new_sha1[20];\n \n \tif (logmsg && read_ref(ref_target, old_sha1))\ndiff --git a/rerere.c b/rerere.c\nindex 8e5532b..02931a1 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -351,7 +351,7 @@ int setup_rerere(struct string_list *merge_rr)\n \tif (!is_rerere_enabled())\n \t\treturn -1;\n \n-\tmerge_rr_path = xstrdup(git_path(\"MERGE_RR\"));\n+\tmerge_rr_path = git_pathdup(\"MERGE_RR\");\n \tfd = hold_lock_file_for_update(&write_lock, merge_rr_path,\n \t\t\t\t       LOCK_DIE_ON_ERROR);\n \tread_rr(merge_rr);\ndiff --git a/server-info.c b/server-info.c\nindex c1c073b..66b0d9d 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -25,7 +25,7 @@ static int add_info_ref(const char *path, const unsigned char *sha1, int flag, v\n \n static int update_info_refs(int force)\n {\n-\tchar *path0 = xstrdup(git_path(\"info/refs\"));\n+\tchar *path0 = git_pathdup(\"info/refs\");\n \tint len = strlen(path0);\n \tchar *path1 = xmalloc(len + 2);\n \n-- \n1.6.0.3.549.gb475d\n\n"},{"id":"94124","messageId":"20081028173132.GB7054@blimp.localdomain","threadId":"16053","inReplyTo":"81b0412b0810280547nf5e6834t4049e92d374926b6@mail.gmail.com","subject":"Re: [PATCH] Add mksnpath and git_snpath which allow to specify the output buffer","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-28T17:31:32Z","receivedAt":"2008-10-28T17:31:32Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Alex Riesen, Tue, Oct 28, 2008 13:47:21 +0100:\n> 2008/10/28 Junio C Hamano <gitster@pobox.com>:\n> > Alex Riesen <raa.lkml@gmail.com> writes:\n> >> Maybe I should resend the patches without it, following by patches\n> >> introducing git_snpath and replacing calls to git_path.\n> >\n> > I took the liberty of doing the first half of just that ;-)\n> >\n> \n> Thanks. And am sorry... I did that too, and stupidly forgot to send.\n> I also considered replacing xstrdup(mkpath) with a function which does\n> just that (patches 8-9). Patches 1 and 2 are unrelated, will send them\n> separately.\n> \n> FWIW now, I'm sending the patches.\n\nErr... The builtin-revert.c hunk of path 0009 depends on the patch I\nsent later: \"Plug a memleak in builtin-revert\". The patch removed const\nin front of \"char *defmsg\".\n"}]}