{"thread":{"id":"24067","subject":"[PATCH next] log_ref_setup: don't return stack-allocated array","startedAt":"2010-06-10T12:43:35Z","lastAt":"2010-06-11T18:54:41Z","messageCount":11,"participants":["Thomas Rast","Erick Mattos","Ævar Arnfjörð Bjarmason","Jay Soffian","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"143423","messageId":"e888313d5a782585f4a5e7ee8914302953c187e2.1276173576.git.trast@student.ethz.ch","threadId":"24067","inReplyTo":null,"subject":"[PATCH next] log_ref_setup: don't return stack-allocated array","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-06-10T12:43:35Z","receivedAt":"2010-06-10T12:43:35Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"859c301 (refs: split log_ref_write logic into log_ref_setup,\n2010-05-21) refactors the stack allocation of the log_file array into\nthe new log_ref_setup() function, but passes it back to the caller.\n\nSince the original intent seems to have been to split the work between\nlog_ref_setup and log_ref_write, make it the caller's responsibility\nto allocate the buffer.\n\nSigned-off-by: Thomas Rast \nReported-by: Ævar Arnfjörð Bjarmason \n---\n\nCauses t5516 to fail, but only if I run it under valgrind. (Ævar\nmanaged to trigger it in other ways apparently.)\n\n refs.c | 10 ++++------\n 1 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3436649..2a3eeec 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1262,13 +1262,11 @@ static int copy_msg(char *buf, const char *msg)\n \treturn cp - buf;\n }\n \n-int log_ref_setup(const char *ref_name, char **log_file)\n+int log_ref_setup(const char *ref_name, char *logfile, int bufsize)\n {\n \tint logfd, oflags = O_APPEND | O_WRONLY;\n-\tchar logfile[PATH_MAX];\n \n-\tgit_snpath(logfile, sizeof(logfile), \"logs/%s\", ref_name);\n-\t*log_file = logfile;\n+\tgit_snpath(logfile, bufsize, \"logs/%s\", ref_name);\n \tif (log_all_ref_updates &&\n \t (!prefixcmp(ref_name, \"refs/heads/\") ||\n \t !prefixcmp(ref_name, \"refs/remotes/\") ||\n@@ -1309,14 +1307,14 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n \tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n \tunsigned maxlen, len;\n \tint msglen;\n-\tchar *log_file;\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-\tresult = log_ref_setup(ref_name, &log_file);\n+\tresult = log_ref_setup(ref_name, log_file, sizeof(log_file));\n \tif (result)\n \t\treturn result;\n \n-- \n1.7.1.553.ga798e\n"},{"id":"143425","messageId":"47daf53b6b2cc25cc013c5f2183e309a671dc9d3.1276174233.git.trast@student.ethz.ch","threadId":"24067","inReplyTo":"e888313d5a782585f4a5e7ee8914302953c187e2.1276173576.git.trast@student.ethz.ch","subject":"[PATCH next v2] log_ref_setup: don't return stack-allocated array","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-06-10T12:54:03Z","receivedAt":"2010-06-10T12:54:03Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"859c301 (refs: split log_ref_write logic into log_ref_setup,\n2010-05-21) refactors the stack allocation of the log_file array into\nthe new log_ref_setup() function, but passes it back to the caller.\n\nSince the original intent seems to have been to split the work between\nlog_ref_setup and log_ref_write, make it the caller's responsibility\nto allocate the buffer.\n\nSigned-off-by: Thomas Rast \nReported-by: Ævar Arnfjörð Bjarmason \n---\n\nSorry for the first one, that was completely botched and didn't even\ncompile.\n\nThis one does, and as an added bonus also passes some tests.\n\n builtin/checkout.c | 4 ++--\n refs.c | 26 ++++++++++++--------------\n refs.h | 2 +-\n 3 files changed, 15 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5107eda..1994be9 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -496,12 +496,12 @@ static void update_refs_for_switch(struct checkout_opts *opts,\n \t\tif (opts->new_orphan_branch) {\n \t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n \t\t\t\tint temp;\n-\t\t\t\tchar *log_file;\n+\t\t\t\tchar log_file[PATH_MAX];\n \t\t\t\tchar *ref_name = mkpath(\"refs/heads/%s\", opts->new_orphan_branch);\n \n \t\t\t\ttemp = log_all_ref_updates;\n \t\t\t\tlog_all_ref_updates = 1;\n-\t\t\t\tif (log_ref_setup(ref_name, &log_file)) {\n+\t\t\t\tif (log_ref_setup(ref_name, log_file, sizeof(log_file))) {\n \t\t\t\t\tfprintf(stderr, \"Can not do reflog for '%s'\\n\",\n \t\t\t\t\t opts->new_orphan_branch);\n \t\t\t\t\tlog_all_ref_updates = temp;\ndiff --git a/refs.c b/refs.c\nindex 3436649..6f486ae 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1262,43 +1262,41 @@ static int copy_msg(char *buf, const char *msg)\n \treturn cp - buf;\n }\n \n-int log_ref_setup(const char *ref_name, char **log_file)\n+int log_ref_setup(const char *ref_name, char *logfile, int bufsize)\n {\n \tint logfd, oflags = O_APPEND | O_WRONLY;\n-\tchar logfile[PATH_MAX];\n \n-\tgit_snpath(logfile, sizeof(logfile), \"logs/%s\", ref_name);\n-\t*log_file = logfile;\n+\tgit_snpath(logfile, bufsize, \"logs/%s\", ref_name);\n \tif (log_all_ref_updates &&\n \t (!prefixcmp(ref_name, \"refs/heads/\") ||\n \t !prefixcmp(ref_name, \"refs/remotes/\") ||\n \t !prefixcmp(ref_name, \"refs/notes/\") ||\n \t !strcmp(ref_name, \"HEAD\"))) {\n-\t\tif (safe_create_leading_directories(*log_file) < 0)\n+\t\tif (safe_create_leading_directories(logfile) < 0)\n \t\t\treturn error(\"unable to create directory for %s\",\n-\t\t\t\t *log_file);\n+\t\t\t\t logfile);\n \t\toflags |= O_CREAT;\n \t}\n \n-\tlogfd = open(*log_file, oflags, 0666);\n+\tlogfd = open(logfile, oflags, 0666);\n \tif (logfd < 0) {\n \t\tif (!(oflags & O_CREAT) && errno == ENOENT)\n \t\t\treturn 0;\n \n \t\tif ((oflags & O_CREAT) && errno == EISDIR) {\n-\t\t\tif (remove_empty_directories(*log_file)) {\n+\t\t\tif (remove_empty_directories(logfile)) {\n \t\t\t\treturn error(\"There are still logs under '%s'\",\n-\t\t\t\t\t *log_file);\n+\t\t\t\t\t logfile);\n \t\t\t}\n-\t\t\tlogfd = open(*log_file, oflags, 0666);\n+\t\t\tlogfd = open(logfile, oflags, 0666);\n \t\t}\n \n \t\tif (logfd < 0)\n \t\t\treturn error(\"Unable to append to %s: %s\",\n-\t\t\t\t *log_file, strerror(errno));\n+\t\t\t\t logfile, strerror(errno));\n \t}\n \n-\tadjust_shared_perm(*log_file);\n+\tadjust_shared_perm(logfile);\n \tclose(logfd);\n \treturn 0;\n }\n@@ -1309,14 +1307,14 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n \tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n \tunsigned maxlen, len;\n \tint msglen;\n-\tchar *log_file;\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-\tresult = log_ref_setup(ref_name, &log_file);\n+\tresult = log_ref_setup(ref_name, log_file, sizeof(log_file));\n \tif (result)\n \t\treturn result;\n \ndiff --git a/refs.h b/refs.h\nindex 594c9d9..762ce50 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -69,7 +69,7 @@ extern void unlock_ref(struct ref_lock *lock);\n extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n \n /** Setup reflog before using. **/\n-int log_ref_setup(const char *ref_name, char **log_file);\n+int log_ref_setup(const char *ref_name, char *logfile, int bufsize);\n \n /** Reads log for the value of ref during at_time. **/\n extern int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *sha1, char **msg, unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt);\n-- \n1.7.1.553.ga798e\n"},{"id":"143451","messageId":"AANLkTillDOCNQrpaEiFsFdq6HpU_LlwWI2ELIrEcrWHc@mail.gmail.com","threadId":"24067","inReplyTo":"47daf53b6b2cc25cc013c5f2183e309a671dc9d3.1276174233.git.trast@student.ethz.ch","subject":"Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-06-10T16:48:40Z","receivedAt":"2010-06-10T16:48:40Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi there,\n\n2010/6/10 Thomas Rast \n>\n> 859c301 (refs: split log_ref_write logic into log_ref_setup,\n> 2010-05-21) refactors the stack allocation of the log_file array into\n> the new log_ref_setup() function, but passes it back to the caller.\n>\n> Since the original intent seems to have been to split the work between\n> log_ref_setup and log_ref_write, make it the caller's responsibility\n> to allocate the buffer.\n>\n> Signed-off-by: Thomas Rast \n> Reported-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n> Sorry for the first one, that was completely botched and didn't even\n> compile.\n>\n> This one does, and as an added bonus also passes some tests.\n>\n>  builtin/checkout.c |    4 ++--\n>  refs.c             |   26 ++++++++++++--------------\n>  refs.h             |    2 +-\n>  3 files changed, 15 insertions(+), 17 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 5107eda..1994be9 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -496,12 +496,12 @@ static void update_refs_for_switch(struct checkout_opts *opts,\n>                if (opts->new_orphan_branch) {\n>                        if (opts->new_branch_log && !log_all_ref_updates) {\n>                                int temp;\n> -                               char *log_file;\n> +                               char log_file[PATH_MAX];\n>                                char *ref_name = mkpath(\"refs/heads/%s\", opts->new_orphan_branch);\n>\n>                                temp = log_all_ref_updates;\n>                                log_all_ref_updates = 1;\n> -                               if (log_ref_setup(ref_name, &log_file)) {\n> +                               if (log_ref_setup(ref_name, log_file, sizeof(log_file))) {\n>                                        fprintf(stderr, \"Can not do reflog for '%s'\\n\",\n>                                            opts->new_orphan_branch);\n>                                        log_all_ref_updates = temp;\n> diff --git a/refs.c b/refs.c\n> index 3436649..6f486ae 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1262,43 +1262,41 @@ static int copy_msg(char *buf, const char *msg)\n>        return cp - buf;\n>  }\n>\n> -int log_ref_setup(const char *ref_name, char **log_file)\n> +int log_ref_setup(const char *ref_name, char *logfile, int bufsize)\n>  {\n>        int logfd, oflags = O_APPEND | O_WRONLY;\n> -       char logfile[PATH_MAX];\n>\n> -       git_snpath(logfile, sizeof(logfile), \"logs/%s\", ref_name);\n> -       *log_file = logfile;\n> +       git_snpath(logfile, bufsize, \"logs/%s\", ref_name);\n>        if (log_all_ref_updates &&\n>            (!prefixcmp(ref_name, \"refs/heads/\") ||\n>             !prefixcmp(ref_name, \"refs/remotes/\") ||\n>             !prefixcmp(ref_name, \"refs/notes/\") ||\n>             !strcmp(ref_name, \"HEAD\"))) {\n> -               if (safe_create_leading_directories(*log_file) < 0)\n> +               if (safe_create_leading_directories(logfile) < 0)\n>                        return error(\"unable to create directory for %s\",\n> -                                    *log_file);\n> +                                    logfile);\n>                oflags |= O_CREAT;\n>        }\n>\n> -       logfd = open(*log_file, oflags, 0666);\n> +       logfd = open(logfile, oflags, 0666);\n>        if (logfd < 0) {\n>                if (!(oflags & O_CREAT) && errno == ENOENT)\n>                        return 0;\n>\n>                if ((oflags & O_CREAT) && errno == EISDIR) {\n> -                       if (remove_empty_directories(*log_file)) {\n> +                       if (remove_empty_directories(logfile)) {\n>                                return error(\"There are still logs under '%s'\",\n> -                                            *log_file);\n> +                                            logfile);\n>                        }\n> -                       logfd = open(*log_file, oflags, 0666);\n> +                       logfd = open(logfile, oflags, 0666);\n>                }\n>\n>                if (logfd < 0)\n>                        return error(\"Unable to append to %s: %s\",\n> -                                    *log_file, strerror(errno));\n> +                                    logfile, strerror(errno));\n>        }\n>\n> -       adjust_shared_perm(*log_file);\n> +       adjust_shared_perm(logfile);\n>        close(logfd);\n>        return 0;\n>  }\n> @@ -1309,14 +1307,14 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n>        int logfd, result, written, oflags = O_APPEND | O_WRONLY;\n>        unsigned maxlen, len;\n>        int msglen;\n> -       char *log_file;\n> +       char log_file[PATH_MAX];\n>        char *logrec;\n>        const char *committer;\n>\n>        if (log_all_ref_updates < 0)\n>                log_all_ref_updates = !is_bare_repository();\n>\n> -       result = log_ref_setup(ref_name, &log_file);\n> +       result = log_ref_setup(ref_name, log_file, sizeof(log_file));\n>        if (result)\n>                return result;\n>\n> diff --git a/refs.h b/refs.h\n> index 594c9d9..762ce50 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -69,7 +69,7 @@ extern void unlock_ref(struct ref_lock *lock);\n>  extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n>\n>  /** Setup reflog before using. **/\n> -int log_ref_setup(const char *ref_name, char **log_file);\n> +int log_ref_setup(const char *ref_name, char *logfile, int bufsize);\n>\n>  /** Reads log for the value of ref during at_time. **/\n>  extern int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *sha1, char **msg, unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt);\n> --\n> 1.7.1.553.ga798e\n\n\nI can't get your point.\n\nI don't see any improvement here.  Unless you want to get rid of using\nreferences on calling functions which is only going to add another\nbuffer to the stack, sized PATH_MAX, once that log_file is going to be\nreally allocated in the heap after git_snpath().  As folks use to say\nhere: \"changing six by half a dozen\".\n\nEven though I think you will need to turn static log_file or to call\ngit_snpath again in the calling functions for this proposed change,\ndon't you?\n\n> Causes t5516 to fail, but only if I run it under valgrind.  (Ævar\n> managed to trigger it in other ways apparently.)\n\nI haven't ever seen this happening so I think you have found some\nparticularity of valgrind which could route a patch to it.\n\nKind regards\n"},{"id":"143457","messageId":"201006101929.11034.trast@student.ethz.ch","threadId":"24067","inReplyTo":"AANLkTillDOCNQrpaEiFsFdq6HpU_LlwWI2ELIrEcrWHc@mail.gmail.com","subject":"Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-06-10T17:29:10Z","receivedAt":"2010-06-10T17:29:10Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Erick Mattos wrote:\n> 2010/6/10 Thomas Rast \n> > -int log_ref_setup(const char *ref_name, char **log_file)\n> > +int log_ref_setup(const char *ref_name, char *logfile, int bufsize)\n> > {\n> > int logfd, oflags = O_APPEND | O_WRONLY;\n> > - char logfile[PATH_MAX];\n> >\n> > - git_snpath(logfile, sizeof(logfile), \"logs/%s\", ref_name);\n> > - *log_file = logfile;\n> > + git_snpath(logfile, bufsize, \"logs/%s\", ref_name);\n[...]\n> I don't see any improvement here. Unless you want to get rid of using\n> references on calling functions which is only going to add another\n> buffer to the stack, sized PATH_MAX, once that log_file is going to be\n> really allocated in the heap after git_snpath(). As folks use to say\n> here: \"changing six by half a dozen\".\n\nWhat the - side of the hunk above does is returning a local (stack\nallocated) variable, in the form of a pointer to logfile. Once those\ngo out of scope, you have zero guarantees on what happens with them.\nTry the following snippet, it should cause a similar problem:\n\n #include \n\n int* f()\n {\n \tint i;\n \ti = 42;\n \treturn &i;\n }\n\n int main()\n {\n \tint *p = f();\n \tif (1) {\n \t\tchar buf[1024];\n \t\tmemset(buf, 0, sizeof(buf));\n \t}\n \tprintf(\"I got: %d\\n\", *p);\n }\n\nOnly in this case the issue is so obvious that the compiler will warn\n(at least mine does).\n\n> I haven't ever seen this happening so I think you have found some\n> particularity of valgrind which could route a patch to it.\n\nAdmittedly my experience is somewhat limited since I don't do C coding\noutside of git and some teaching. But so far I have not had a single\nfalse alarm with valgrind (when compiled without optimizations;\notherwise the compiler may do some magic).\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"143462","messageId":"AANLkTimPCMbprIKQ__SfMej3oST5agPZ06hM2dkyiUfj@mail.gmail.com","threadId":"24067","inReplyTo":"AANLkTinI44rPfeXvWr-7jvAVyw5itX_gUsHimwSL74Lv@mail.gmail.com","subject":"Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-06-10T18:09:26Z","receivedAt":"2010-06-10T18:09:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Jun 10, 2010 at 16:46, Erick Mattos wrote:\n> Hi there,\n>\n> 2010/6/10 Thomas Rast \n>>\n>> 859c301 (refs: split log_ref_write logic into log_ref_setup,\n>> 2010-05-21) refactors the stack allocation of the log_file array into\n>> the new log_ref_setup() function, but passes it back to the caller.\n>>\n>> Since the original intent seems to have been to split the work between\n>> log_ref_setup and log_ref_write, make it the caller's responsibility\n>> to allocate the buffer.\n>>\n>> Signed-off-by: Thomas Rast \n>> Reported-by: Ævar Arnfjörð Bjarmason \n>> ---\n>>\n>> Sorry for the first one, that was completely botched and didn't even\n>> compile.\n>>\n>> This one does, and as an added bonus also passes some tests.\n>>\n>>  builtin/checkout.c |    4 ++--\n>>  refs.c             |   26 ++++++++++++--------------\n>>  refs.h             |    2 +-\n>>  3 files changed, 15 insertions(+), 17 deletions(-)\n>>\n>> diff --git a/builtin/checkout.c b/builtin/checkout.c\n>> index 5107eda..1994be9 100644\n>> --- a/builtin/checkout.c\n>> +++ b/builtin/checkout.c\n>> @@ -496,12 +496,12 @@ static void update_refs_for_switch(struct\n>> checkout_opts *opts,\n>>                if (opts->new_orphan_branch) {\n>>                        if (opts->new_branch_log && !log_all_ref_updates) {\n>>                                int temp;\n>> -                               char *log_file;\n>> +                               char log_file[PATH_MAX];\n>>                                char *ref_name = mkpath(\"refs/heads/%s\",\n>> opts->new_orphan_branch);\n>>\n>>                                temp = log_all_ref_updates;\n>>                                log_all_ref_updates = 1;\n>> -                               if (log_ref_setup(ref_name, &log_file)) {\n>> +                               if (log_ref_setup(ref_name, log_file,\n>> sizeof(log_file))) {\n>>                                        fprintf(stderr, \"Can not do reflog\n>> for '%s'\\n\",\n>>                                            opts->new_orphan_branch);\n>>                                        log_all_ref_updates = temp;\n>> diff --git a/refs.c b/refs.c\n>> index 3436649..6f486ae 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1262,43 +1262,41 @@ static int copy_msg(char *buf, const char *msg)\n>>        return cp - buf;\n>>  }\n>>\n>> -int log_ref_setup(const char *ref_name, char **log_file)\n>> +int log_ref_setup(const char *ref_name, char *logfile, int bufsize)\n>>  {\n>>        int logfd, oflags = O_APPEND | O_WRONLY;\n>> -       char logfile[PATH_MAX];\n>>\n>> -       git_snpath(logfile, sizeof(logfile), \"logs/%s\", ref_name);\n>> -       *log_file = logfile;\n>> +       git_snpath(logfile, bufsize, \"logs/%s\", ref_name);\n>>        if (log_all_ref_updates &&\n>>            (!prefixcmp(ref_name, \"refs/heads/\") ||\n>>             !prefixcmp(ref_name, \"refs/remotes/\") ||\n>>             !prefixcmp(ref_name, \"refs/notes/\") ||\n>>             !strcmp(ref_name, \"HEAD\"))) {\n>> -               if (safe_create_leading_directories(*log_file) < 0)\n>> +               if (safe_create_leading_directories(logfile) < 0)\n>>                        return error(\"unable to create directory for %s\",\n>> -                                    *log_file);\n>> +                                    logfile);\n>>                oflags |= O_CREAT;\n>>        }\n>>\n>> -       logfd = open(*log_file, oflags, 0666);\n>> +       logfd = open(logfile, oflags, 0666);\n>>        if (logfd < 0) {\n>>                if (!(oflags & O_CREAT) && errno == ENOENT)\n>>                        return 0;\n>>\n>>                if ((oflags & O_CREAT) && errno == EISDIR) {\n>> -                       if (remove_empty_directories(*log_file)) {\n>> +                       if (remove_empty_directories(logfile)) {\n>>                                return error(\"There are still logs under\n>> '%s'\",\n>> -                                            *log_file);\n>> +                                            logfile);\n>>                        }\n>> -                       logfd = open(*log_file, oflags, 0666);\n>> +                       logfd = open(logfile, oflags, 0666);\n>>                }\n>>\n>>                if (logfd < 0)\n>>                        return error(\"Unable to append to %s: %s\",\n>> -                                    *log_file, strerror(errno));\n>> +                                    logfile, strerror(errno));\n>>        }\n>>\n>> -       adjust_shared_perm(*log_file);\n>> +       adjust_shared_perm(logfile);\n>>        close(logfd);\n>>        return 0;\n>>  }\n>> @@ -1309,14 +1307,14 @@ static int log_ref_write(const char *ref_name,\n>> const unsigned char *old_sha1,\n>>        int logfd, result, written, oflags = O_APPEND | O_WRONLY;\n>>        unsigned maxlen, len;\n>>        int msglen;\n>> -       char *log_file;\n>> +       char log_file[PATH_MAX];\n>>        char *logrec;\n>>        const char *committer;\n>>\n>>        if (log_all_ref_updates < 0)\n>>                log_all_ref_updates = !is_bare_repository();\n>>\n>> -       result = log_ref_setup(ref_name, &log_file);\n>> +       result = log_ref_setup(ref_name, log_file, sizeof(log_file));\n>>        if (result)\n>>                return result;\n>>\n>> diff --git a/refs.h b/refs.h\n>> index 594c9d9..762ce50 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -69,7 +69,7 @@ extern void unlock_ref(struct ref_lock *lock);\n>>  extern int write_ref_sha1(struct ref_lock *lock, const unsigned char\n>> *sha1, const char *msg);\n>>\n>>  /** Setup reflog before using. **/\n>> -int log_ref_setup(const char *ref_name, char **log_file);\n>> +int log_ref_setup(const char *ref_name, char *logfile, int bufsize);\n>>\n>>  /** Reads log for the value of ref during at_time. **/\n>>  extern int read_ref_at(const char *ref, unsigned long at_time, int cnt,\n>> unsigned char *sha1, char **msg, unsigned long *cutoff_time, int *cutoff_tz,\n>> int *cutoff_cnt);\n>> --\n>> 1.7.1.553.ga798e\n>>\n>\n> I can't get your point.\n>\n> I don't see any improvement here.  Unless you want to get rid of using\n> references on calling functions which is only going to add another buffer to\n> the stack, sized PATH_MAX, once that log_file is going to be really\n> allocated in the heap after git_snpath().  As folks use to say here:\n> \"changing six by half a dozen\".\n>\n> Even though I think you will need to turn static log_file or to call\n> git_snpath again in the calling functions for this proposed change, don't\n> you?\n>\n>> Causes t5516 to fail, but only if I run it under valgrind.  (Ævar\n>> managed to trigger it in other ways apparently.)\n>\n> I haven't ever seen this happening so I think you have found some\n> particularity of valgrind which could route a patch to it.\n\nActually I think my test failure is related to da3efdb17b, see the\n\"[PATCH v2 2/2] receive-pack: detect aliased updates which can occur\nwith symrefs\" thread.\n\nThe patch under discussion here might be good, I don't have the\nknowledge to review it. But it's probably not what's causing the issue\ni had with t5516.\n"},{"id":"143469","messageId":"f99f845d5d0aa77b0a95c35f9289f1b031897d43.1276195180.git.trast@student.ethz.ch","threadId":"24067","inReplyTo":"AANLkTimPCMbprIKQ__SfMej3oST5agPZ06hM2dkyiUfj@mail.gmail.com","subject":"[PATCH] check_aliased_update: strcpy() instead of strcat() to copy","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-06-10T18:43:51Z","receivedAt":"2010-06-10T18:43:51Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"da3efdb (receive-pack: detect aliased updates which can occur with\nsymrefs, 2010-04-19) introduced two strcat() into uninitialized\nstrings. The intent was clearly make a copy of the static buffer used\nby find_unique_abbrev(), so use strcpy() instead.\n\nSigned-off-by: Thomas Rast \nReported-by: Ævar Arnfjörð Bjarmason \n---\n\n> Actually I think my test failure is related to da3efdb17b, see the\n> \"[PATCH v2 2/2] receive-pack: detect aliased updates which can occur\n> with symrefs\" thread.\n\nIndeed, there's another bug in this one. (And valgrind catches it\ntoo... if only I had the patience to let it churn through t5516!)\n\nUnlike the other bug, this one is already in master.\n\n builtin/receive-pack.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bb34757..7e4129d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -515,9 +515,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \tdst_cmd->skip_update = 1;\n \n \tstrcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));\n-\tstrcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));\n+\tstrcpy(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));\n \tstrcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));\n-\tstrcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));\n+\tstrcpy(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));\n \trp_error(\"refusing inconsistent update between symref '%s' (%s..%s) and\"\n \t\t \" its target '%s' (%s..%s)\",\n \t\t cmd->ref_name, cmd_oldh, cmd_newh,\n-- \n1.7.1.561.g94582\n"},{"id":"143471","messageId":"AANLkTin8kU-Ods3GV3ZpetSdpu0tisipDuWzlqdiCLPT@mail.gmail.com","threadId":"24067","inReplyTo":"f99f845d5d0aa77b0a95c35f9289f1b031897d43.1276195180.git.trast@student.ethz.ch","subject":"Re: [PATCH] check_aliased_update: strcpy() instead of strcat() to copy","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-06-10T19:00:16Z","receivedAt":"2010-06-10T19:00:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Jun 10, 2010 at 18:43, Thomas Rast wrote:\n> da3efdb (receive-pack: detect aliased updates which can occur with\n> symrefs, 2010-04-19) introduced two strcat() into uninitialized\n> strings.  The intent was clearly make a copy of the static buffer used\n> by find_unique_abbrev(), so use strcpy() instead.\n>\n> Signed-off-by: Thomas Rast \n> Reported-by: Ævar Arnfjörð Bjarmason \n\nTested-by: Ævar Arnfjörð Bjarmason \n\nThis fixes the problem I was having. Thanks.\n"},{"id":"143474","messageId":"AANLkTikNyyIk2952ei2kXsQJcznunmDJ30Ze2Sjb8V2M@mail.gmail.com","threadId":"24067","inReplyTo":"f99f845d5d0aa77b0a95c35f9289f1b031897d43.1276195180.git.trast@student.ethz.ch","subject":"Re: [PATCH] check_aliased_update: strcpy() instead of strcat() to copy","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-06-10T19:26:16Z","receivedAt":"2010-06-10T19:26:16Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Jun 10, 2010 at 2:43 PM, Thomas Rast wrote:\n> da3efdb (receive-pack: detect aliased updates which can occur with\n> symrefs, 2010-04-19) introduced two strcat() into uninitialized\n> strings.  The intent was clearly make a copy of the static buffer used\n> by find_unique_abbrev(), so use strcpy() instead.\n>\n> Signed-off-by: Thomas Rast \n> Reported-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n>> Actually I think my test failure is related to da3efdb17b, see the\n>> \"[PATCH v2 2/2] receive-pack: detect aliased updates which can occur\n>> with symrefs\" thread.\n>\n> Indeed, there's another bug in this one.  (And valgrind catches it\n> too...  if only I had the patience to let it churn through t5516!)\n>\n> Unlike the other bug, this one is already in master.\n>\n>  builtin/receive-pack.c |    4 ++--\n>  1 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index bb34757..7e4129d 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -515,9 +515,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n>        dst_cmd->skip_update = 1;\n>\n>        strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));\n> -       strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));\n> +       strcpy(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));\n>        strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));\n> -       strcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));\n> +       strcpy(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));\n>        rp_error(\"refusing inconsistent update between symref '%s' (%s..%s) and\"\n>                 \" its target '%s' (%s..%s)\",\n>                 cmd->ref_name, cmd_oldh, cmd_newh,\n\nThanks. I cannot imagine what I was thinking. Maybe a cut-and-paste\nerror from somewhere else. I am sad this made it all the way to\nmaster.\n\nj.\n"},{"id":"143481","messageId":"AANLkTimEwV_bJkd_2csJB0L6T9Lq6F0hpllUO2pJTL8m@mail.gmail.com","threadId":"24067","inReplyTo":"201006101929.11034.trast@student.ethz.ch","subject":"Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-06-10T23:09:36Z","receivedAt":"2010-06-10T23:09:36Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi,\n\nWe are becoming a little theoretical here so people please be\ncondescending to us if the chat gets a little boring. ;-)\n\n2010/6/10 Thomas Rast :\n> What the - side of the hunk above does is returning a local (stack\n> allocated) variable, in the form of a pointer to logfile.  Once those\n> go out of scope, you have zero guarantees on what happens with them.\n\nNot really.\n\nWhat the actual log_ref_setup() does when is instantiated is to create\na pointer in the stack, called log_file, to a pointer to a char array.\n This pointer receives the address of a char array of the calling\nfunction because that is why passing by reference is made to. See\nthat the calling functions is using the \"&\" when making the call (If I\nwas using C++ I would pass by reference the array itself but in C I\ncan only pass pointer variables by reference that is why the pointer\nto a pointer).\n\nThen git_snpath() creates a char array in the heap with the right\ncontent and changes the stack pointer logfile to it. Then when we do\n*log_file = logfile what is happening is that the content of the\npointer in the stack, log_file, which its content is the calling\nfunction char array pointer, is being set to the address of the buffer\ncreated in the heap by git_snpath(). After it, what you have is just\nthe natural cleanup of the stack variables of the called function but\nthe calling variable keeps the address of the char array in the heap\ncreated by git_snpath().\n\n> Try the following snippet, it should cause a similar problem:\n>\n>  #include \n>\n>  int* f()\n>  {\n>        int i;\n>        i = 42;\n>        return &i;\n>  }\n>\n>  int main()\n>  {\n>        int *p = f();\n>        if (1) {\n>                char buf[1024];\n>                memset(buf, 0, sizeof(buf));\n>        }\n>        printf(\"I got: %d\\n\", *p);\n>  }\n>\n> Only in this case the issue is so obvious that the compiler will warn\n> (at least mine does).\n\nThat is another thing.\n\n>> I haven't ever seen this happening so I think you have found some\n>> particularity of valgrind which could route a patch to it.\n>\n> Admittedly my experience is somewhat limited since I don't do C coding\n> outside of git and some teaching.  But so far I have not had a single\n> false alarm with valgrind (when compiled without optimizations;\n> otherwise the compiler may do some magic).\n\nI don't think I am good enough too. I have always things to learn.\nAnd I hopefully will be always learning new things! I am addicted to\nit. :-S\n\nEverybody is here to help each other and NOBODY does not commit\nmistakes so your help is very welcome but don't deposit all your\nbeliefs in any piece of software because all of them could contain\nmistakes too, even if the best ever existent programmers had made them\nin whole.\n\nProgramming is teaching a limited dumb equipment to be clever. So it\nis in itself a contradiction.\n\nBest regards\n"},{"id":"143485","messageId":"20100611051236.GA3947@coredump.intra.peff.net","threadId":"24067","inReplyTo":"AANLkTimEwV_bJkd_2csJB0L6T9Lq6F0hpllUO2pJTL8m@mail.gmail.com","subject":"Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-11T05:12:36Z","receivedAt":"2010-06-11T05:12:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 10, 2010 at 08:09:36PM -0300, Erick Mattos wrote:\n\n> 2010/6/10 Thomas Rast :\n> > What the - side of the hunk above does is returning a local (stack\n> > allocated) variable, in the form of a pointer to logfile.  Once those\n> > go out of scope, you have zero guarantees on what happens with them.\n> \n> Not really.\n> \n> What the actual log_ref_setup() does when is instantiated is to create\n> a pointer in the stack, called log_file, to a pointer to a char array.\n> This pointer receives the address of a char array of the calling\n> function because that is why passing by reference is made to. See\n> that the calling functions is using the \"&\" when making the call (If I\n> was using C++ I would pass by reference the array itself but in C I\n> can only pass pointer variables by reference that is why the pointer\n> to a pointer).\n\nNo, Thomas is right. This invokes undefined behavior. We point the\npassed-in log_file pointer to the front of a character array with\nautomatic duration. After log_ref_setup returns, we must never\ndereference that pointer again, but we do. So we need this patch or\nsomething like it.\n\nIn practice, it worked because allocating on the stack is really just\nabout bumping the stack pointer, so that memory sits there until another\nfunction call needs it for stack variables. After returning from\nlog_ref_setup, we don't actually make any other function calls before\ncalling open(log_file), so the buffer was still there, untouched. There\nis a later use of log_file which is probably bogus, but was likely never\ntriggered because it is in an unlikely error conditional.\n\n> Then git_snpath() creates a char array in the heap with the right\n> content and changes the stack pointer logfile to it. Then when we do\n\nNo, it doesn't. git_snpath writes into the buffer you provide it, just\nlike snprintf (hence the name).\n\n> > Admittedly my experience is somewhat limited since I don't do C coding\n> > outside of git and some teaching.  But so far I have not had a single\n> > false alarm with valgrind (when compiled without optimizations;\n> > otherwise the compiler may do some magic).\n\nWe have some false positives in git, but you don't see them because\nt/valgrind/default.supp suppresses them. For example:\n\n http://thread.gmane.org/gmane.comp.version-control.git/106335/focus=107302\n\nIf you are using a binary package of valgrind, it probably ships with\nsome system-specific suppressions, too. Right now valgrind on Debian\nunstable is next to useless because glibc has been upgraded to 2.11, but\nthe suppressions haven't been updated. So you get false positives all\nover the place because of clever architecture-specific optimizations\n(e.g., I am seeing a lot of __strlen_sse2 problems, which are probably\njust the function over-reading its input data because processing big\nchunks is faster).\n\n-Peff\n"},{"id":"143525","messageId":"AANLkTikhgl2b_66POXPf1nJSlhwkY5PV1Qce3cA9yXOx@mail.gmail.com","threadId":"24067","inReplyTo":"20100611051236.GA3947@coredump.intra.peff.net","subject":"Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array","fromName":"Erick Mattos","fromEmail":"erick.mattos@gmail.com","sentAt":"2010-06-11T18:54:41Z","receivedAt":"2010-06-11T18:54:41Z","isPatch":true,"sender":{"key":"erick.mattos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/134001?v=4"},"body":"Hi,\n\n2010/6/11 Jeff King :\n> No, Thomas is right. This invokes undefined behavior. We point the\n> passed-in log_file pointer to the front of a character array with\n> automatic duration. After log_ref_setup returns, we must never\n> dereference that pointer again, but we do. So we need this patch or\n> something like it.\n\nYou and Thomas are right on this subject. I don't know when and how I\ncould see a malloc() in git_(v)snpath(). My fault.\n\n>> Then git_snpath() creates a char array in the heap with the right\n>> content and changes the stack pointer logfile to it.  Then when we do\n>\n> No, it doesn't. git_snpath writes into the buffer you provide it, just\n> like snprintf (hence the name).\n\nThe source of my failure.\n\n> We have some false positives in git, but you don't see them because\n> t/valgrind/default.supp suppresses them. For example:\n>\n>  http://thread.gmane.org/gmane.comp.version-control.git/106335/focus=107302\n>\n> If you are using a binary package of valgrind, it probably ships with\n> some system-specific suppressions, too. Right now valgrind on Debian\n> unstable is next to useless because glibc has been upgraded to 2.11, but\n> the suppressions haven't been updated. So you get false positives all\n> over the place because of clever architecture-specific optimizations\n> (e.g., I am seeing a lot of __strlen_sse2 problems, which are probably\n> just the function over-reading its input data because processing big\n> chunks is faster).\n>\n> -Peff\n\nThanks for the extended explanation about valgrind.\n\nRegards to all\n"}]}