[PATCH next] log_ref_setup: don't return stack-allocated array

[PATCH next] log_ref_setup: don't return stack-allocated array

[Thomas Rast]

859c301 (refs: split log_ref_write logic into log_ref_setup,
2010-05-21) refactors the stack allocation of the log_file array into
the new log_ref_setup() function, but passes it back to the caller.

Since the original intent seems to have been to split the work between
log_ref_setup and log_ref_write, make it the caller's responsibility
to allocate the buffer.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Causes t5516 to fail, but only if I run it under valgrind.  (Ævar
managed to trigger it in other ways apparently.)

 refs.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 3436649..2a3eeec 100644
--- a/refs.c
+++ b/refs.c
@@ -1262,13 +1262,11 @@ static int copy_msg(char *buf, const char *msg)
 	return cp - buf;
 }
 
-int log_ref_setup(const char *ref_name, char **log_file)
+int log_ref_setup(const char *ref_name, char *logfile, int bufsize)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
-	char logfile[PATH_MAX];
 
-	git_snpath(logfile, sizeof(logfile), "logs/%s", ref_name);
-	*log_file = logfile;
+	git_snpath(logfile, bufsize, "logs/%s", ref_name);
 	if (log_all_ref_updates &&
 	    (!prefixcmp(ref_name, "refs/heads/") ||
 	     !prefixcmp(ref_name, "refs/remotes/") ||
@@ -1309,14 +1307,14 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
 	unsigned maxlen, len;
 	int msglen;
-	char *log_file;
+	char log_file[PATH_MAX];
 	char *logrec;
 	const char *committer;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(ref_name, &log_file);
+	result = log_ref_setup(ref_name, log_file, sizeof(log_file));
 	if (result)
 		return result;
 
-- 
1.7.1.553.ga798e

[PATCH next v2] log_ref_setup: don't return stack-allocated array

[Thomas Rast]

859c301 (refs: split log_ref_write logic into log_ref_setup,
2010-05-21) refactors the stack allocation of the log_file array into
the new log_ref_setup() function, but passes it back to the caller.

Since the original intent seems to have been to split the work between
log_ref_setup and log_ref_write, make it the caller's responsibility
to allocate the buffer.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Sorry for the first one, that was completely botched and didn't even
compile.

This one does, and as an added bonus also passes some tests.

 builtin/checkout.c |    4 ++--
 refs.c             |   26 ++++++++++++--------------
 refs.h             |    2 +-
 3 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5107eda..1994be9 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -496,12 +496,12 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 		if (opts->new_orphan_branch) {
 			if (opts->new_branch_log && !log_all_ref_updates) {
 				int temp;
-				char *log_file;
+				char log_file[PATH_MAX];
 				char *ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
 
 				temp = log_all_ref_updates;
 				log_all_ref_updates = 1;
-				if (log_ref_setup(ref_name, &log_file)) {
+				if (log_ref_setup(ref_name, log_file, sizeof(log_file))) {
 					fprintf(stderr, "Can not do reflog for '%s'\n",
 					    opts->new_orphan_branch);
 					log_all_ref_updates = temp;
diff --git a/refs.c b/refs.c
index 3436649..6f486ae 100644
--- a/refs.c
+++ b/refs.c
@@ -1262,43 +1262,41 @@ static int copy_msg(char *buf, const char *msg)
 	return cp - buf;
 }
 
-int log_ref_setup(const char *ref_name, char **log_file)
+int log_ref_setup(const char *ref_name, char *logfile, int bufsize)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
-	char logfile[PATH_MAX];
 
-	git_snpath(logfile, sizeof(logfile), "logs/%s", ref_name);
-	*log_file = logfile;
+	git_snpath(logfile, bufsize, "logs/%s", ref_name);
 	if (log_all_ref_updates &&
 	    (!prefixcmp(ref_name, "refs/heads/") ||
 	     !prefixcmp(ref_name, "refs/remotes/") ||
 	     !prefixcmp(ref_name, "refs/notes/") ||
 	     !strcmp(ref_name, "HEAD"))) {
-		if (safe_create_leading_directories(*log_file) < 0)
+		if (safe_create_leading_directories(logfile) < 0)
 			return error("unable to create directory for %s",
-				     *log_file);
+				     logfile);
 		oflags |= O_CREAT;
 	}
 
-	logfd = open(*log_file, oflags, 0666);
+	logfd = open(logfile, oflags, 0666);
 	if (logfd < 0) {
 		if (!(oflags & O_CREAT) && errno == ENOENT)
 			return 0;
 
 		if ((oflags & O_CREAT) && errno == EISDIR) {
-			if (remove_empty_directories(*log_file)) {
+			if (remove_empty_directories(logfile)) {
 				return error("There are still logs under '%s'",
-					     *log_file);
+					     logfile);
 			}
-			logfd = open(*log_file, oflags, 0666);
+			logfd = open(logfile, oflags, 0666);
 		}
 
 		if (logfd < 0)
 			return error("Unable to append to %s: %s",
-				     *log_file, strerror(errno));
+				     logfile, strerror(errno));
 	}
 
-	adjust_shared_perm(*log_file);
+	adjust_shared_perm(logfile);
 	close(logfd);
 	return 0;
 }
@@ -1309,14 +1307,14 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
 	unsigned maxlen, len;
 	int msglen;
-	char *log_file;
+	char log_file[PATH_MAX];
 	char *logrec;
 	const char *committer;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(ref_name, &log_file);
+	result = log_ref_setup(ref_name, log_file, sizeof(log_file));
 	if (result)
 		return result;
 
diff --git a/refs.h b/refs.h
index 594c9d9..762ce50 100644
--- a/refs.h
+++ b/refs.h
@@ -69,7 +69,7 @@ extern void unlock_ref(struct ref_lock *lock);
 extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
 
 /** Setup reflog before using. **/
-int log_ref_setup(const char *ref_name, char **log_file);
+int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
 
 /** Reads log for the value of ref during at_time. **/
 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);
-- 
1.7.1.553.ga798e

Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array

[Erick Mattos]

Hi there,

2010/6/10 Thomas Rast <trast@student.ethz.ch>
>
> 859c301 (refs: split log_ref_write logic into log_ref_setup,
> 2010-05-21) refactors the stack allocation of the log_file array into
> the new log_ref_setup() function, but passes it back to the caller.
>
> Since the original intent seems to have been to split the work between
> log_ref_setup and log_ref_write, make it the caller's responsibility
> to allocate the buffer.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> Sorry for the first one, that was completely botched and didn't even
> compile.
>
> This one does, and as an added bonus also passes some tests.
>
>  builtin/checkout.c |    4 ++--
>  refs.c             |   26 ++++++++++++--------------
>  refs.h             |    2 +-
>  3 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5107eda..1994be9 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -496,12 +496,12 @@ static void update_refs_for_switch(struct checkout_opts *opts,
>                if (opts->new_orphan_branch) {
>                        if (opts->new_branch_log && !log_all_ref_updates) {
>                                int temp;
> -                               char *log_file;
> +                               char log_file[PATH_MAX];
>                                char *ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
>
>                                temp = log_all_ref_updates;
>                                log_all_ref_updates = 1;
> -                               if (log_ref_setup(ref_name, &log_file)) {
> +                               if (log_ref_setup(ref_name, log_file, sizeof(log_file))) {
>                                        fprintf(stderr, "Can not do reflog for '%s'\n",
>                                            opts->new_orphan_branch);
>                                        log_all_ref_updates = temp;
> diff --git a/refs.c b/refs.c
> index 3436649..6f486ae 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1262,43 +1262,41 @@ static int copy_msg(char *buf, const char *msg)
>        return cp - buf;
>  }
>
> -int log_ref_setup(const char *ref_name, char **log_file)
> +int log_ref_setup(const char *ref_name, char *logfile, int bufsize)
>  {
>        int logfd, oflags = O_APPEND | O_WRONLY;
> -       char logfile[PATH_MAX];
>
> -       git_snpath(logfile, sizeof(logfile), "logs/%s", ref_name);
> -       *log_file = logfile;
> +       git_snpath(logfile, bufsize, "logs/%s", ref_name);
>        if (log_all_ref_updates &&
>            (!prefixcmp(ref_name, "refs/heads/") ||
>             !prefixcmp(ref_name, "refs/remotes/") ||
>             !prefixcmp(ref_name, "refs/notes/") ||
>             !strcmp(ref_name, "HEAD"))) {
> -               if (safe_create_leading_directories(*log_file) < 0)
> +               if (safe_create_leading_directories(logfile) < 0)
>                        return error("unable to create directory for %s",
> -                                    *log_file);
> +                                    logfile);
>                oflags |= O_CREAT;
>        }
>
> -       logfd = open(*log_file, oflags, 0666);
> +       logfd = open(logfile, oflags, 0666);
>        if (logfd < 0) {
>                if (!(oflags & O_CREAT) && errno == ENOENT)
>                        return 0;
>
>                if ((oflags & O_CREAT) && errno == EISDIR) {
> -                       if (remove_empty_directories(*log_file)) {
> +                       if (remove_empty_directories(logfile)) {
>                                return error("There are still logs under '%s'",
> -                                            *log_file);
> +                                            logfile);
>                        }
> -                       logfd = open(*log_file, oflags, 0666);
> +                       logfd = open(logfile, oflags, 0666);
>                }
>
>                if (logfd < 0)
>                        return error("Unable to append to %s: %s",
> -                                    *log_file, strerror(errno));
> +                                    logfile, strerror(errno));
>        }
>
> -       adjust_shared_perm(*log_file);
> +       adjust_shared_perm(logfile);
>        close(logfd);
>        return 0;
>  }
> @@ -1309,14 +1307,14 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
>        int logfd, result, written, oflags = O_APPEND | O_WRONLY;
>        unsigned maxlen, len;
>        int msglen;
> -       char *log_file;
> +       char log_file[PATH_MAX];
>        char *logrec;
>        const char *committer;
>
>        if (log_all_ref_updates < 0)
>                log_all_ref_updates = !is_bare_repository();
>
> -       result = log_ref_setup(ref_name, &log_file);
> +       result = log_ref_setup(ref_name, log_file, sizeof(log_file));
>        if (result)
>                return result;
>
> diff --git a/refs.h b/refs.h
> index 594c9d9..762ce50 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -69,7 +69,7 @@ extern void unlock_ref(struct ref_lock *lock);
>  extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
>
>  /** Setup reflog before using. **/
> -int log_ref_setup(const char *ref_name, char **log_file);
> +int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
>
>  /** Reads log for the value of ref during at_time. **/
>  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);
> --
> 1.7.1.553.ga798e


I can't get your point.

I don't see any improvement here.  Unless you want to get rid of using
references on calling functions which is only going to add another
buffer to the stack, sized PATH_MAX, once that log_file is going to be
really allocated in the heap after git_snpath().  As folks use to say
here: "changing six by half a dozen".

Even though I think you will need to turn static log_file or to call
git_snpath again in the calling functions for this proposed change,
don't you?

> Causes t5516 to fail, but only if I run it under valgrind.  (Ævar
> managed to trigger it in other ways apparently.)

I haven't ever seen this happening so I think you have found some
particularity of valgrind which could route a patch to it.

Kind regards

Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array

[Thomas Rast]

Erick Mattos wrote:
> 2010/6/10 Thomas Rast <trast@student.ethz.ch>
> > -int log_ref_setup(const char *ref_name, char **log_file)
> > +int log_ref_setup(const char *ref_name, char *logfile, int bufsize)
> >  {
> >        int logfd, oflags = O_APPEND | O_WRONLY;
> > -       char logfile[PATH_MAX];
> >
> > -       git_snpath(logfile, sizeof(logfile), "logs/%s", ref_name);
> > -       *log_file = logfile;
> > +       git_snpath(logfile, bufsize, "logs/%s", ref_name);
[...]
> I don't see any improvement here.  Unless you want to get rid of using
> references on calling functions which is only going to add another
> buffer to the stack, sized PATH_MAX, once that log_file is going to be
> really allocated in the heap after git_snpath().  As folks use to say
> here: "changing six by half a dozen".

What the - side of the hunk above does is returning a local (stack
allocated) variable, in the form of a pointer to logfile.  Once those
go out of scope, you have zero guarantees on what happens with them.
Try the following snippet, it should cause a similar problem:

  #include <stdio.h>

  int* f()
  {
  	int i;
  	i = 42;
  	return &i;
  }

  int main()
  {
  	int *p = f();
  	if (1) {
  		char buf[1024];
  		memset(buf, 0, sizeof(buf));
  	}
  	printf("I got: %d\n", *p);
  }

Only in this case the issue is so obvious that the compiler will warn
(at least mine does).

> I haven't ever seen this happening so I think you have found some
> particularity of valgrind which could route a patch to it.

Admittedly my experience is somewhat limited since I don't do C coding
outside of git and some teaching.  But so far I have not had a single
false alarm with valgrind (when compiled without optimizations;
otherwise the compiler may do some magic).

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 10, 2010 at 16:46, Erick Mattos <erick.mattos@gmail.com> wrote:
> Hi there,
>
> 2010/6/10 Thomas Rast <trast@student.ethz.ch>
>>
>> 859c301 (refs: split log_ref_write logic into log_ref_setup,
>> 2010-05-21) refactors the stack allocation of the log_file array into
>> the new log_ref_setup() function, but passes it back to the caller.
>>
>> Since the original intent seems to have been to split the work between
>> log_ref_setup and log_ref_write, make it the caller's responsibility
>> to allocate the buffer.
>>
>> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
>> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> Sorry for the first one, that was completely botched and didn't even
>> compile.
>>
>> This one does, and as an added bonus also passes some tests.
>>
>>  builtin/checkout.c |    4 ++--
>>  refs.c             |   26 ++++++++++++--------------
>>  refs.h             |    2 +-
>>  3 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index 5107eda..1994be9 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -496,12 +496,12 @@ static void update_refs_for_switch(struct
>> checkout_opts *opts,
>>                if (opts->new_orphan_branch) {
>>                        if (opts->new_branch_log && !log_all_ref_updates) {
>>                                int temp;
>> -                               char *log_file;
>> +                               char log_file[PATH_MAX];
>>                                char *ref_name = mkpath("refs/heads/%s",
>> opts->new_orphan_branch);
>>
>>                                temp = log_all_ref_updates;
>>                                log_all_ref_updates = 1;
>> -                               if (log_ref_setup(ref_name, &log_file)) {
>> +                               if (log_ref_setup(ref_name, log_file,
>> sizeof(log_file))) {
>>                                        fprintf(stderr, "Can not do reflog
>> for '%s'\n",
>>                                            opts->new_orphan_branch);
>>                                        log_all_ref_updates = temp;
>> diff --git a/refs.c b/refs.c
>> index 3436649..6f486ae 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1262,43 +1262,41 @@ static int copy_msg(char *buf, const char *msg)
>>        return cp - buf;
>>  }
>>
>> -int log_ref_setup(const char *ref_name, char **log_file)
>> +int log_ref_setup(const char *ref_name, char *logfile, int bufsize)
>>  {
>>        int logfd, oflags = O_APPEND | O_WRONLY;
>> -       char logfile[PATH_MAX];
>>
>> -       git_snpath(logfile, sizeof(logfile), "logs/%s", ref_name);
>> -       *log_file = logfile;
>> +       git_snpath(logfile, bufsize, "logs/%s", ref_name);
>>        if (log_all_ref_updates &&
>>            (!prefixcmp(ref_name, "refs/heads/") ||
>>             !prefixcmp(ref_name, "refs/remotes/") ||
>>             !prefixcmp(ref_name, "refs/notes/") ||
>>             !strcmp(ref_name, "HEAD"))) {
>> -               if (safe_create_leading_directories(*log_file) < 0)
>> +               if (safe_create_leading_directories(logfile) < 0)
>>                        return error("unable to create directory for %s",
>> -                                    *log_file);
>> +                                    logfile);
>>                oflags |= O_CREAT;
>>        }
>>
>> -       logfd = open(*log_file, oflags, 0666);
>> +       logfd = open(logfile, oflags, 0666);
>>        if (logfd < 0) {
>>                if (!(oflags & O_CREAT) && errno == ENOENT)
>>                        return 0;
>>
>>                if ((oflags & O_CREAT) && errno == EISDIR) {
>> -                       if (remove_empty_directories(*log_file)) {
>> +                       if (remove_empty_directories(logfile)) {
>>                                return error("There are still logs under
>> '%s'",
>> -                                            *log_file);
>> +                                            logfile);
>>                        }
>> -                       logfd = open(*log_file, oflags, 0666);
>> +                       logfd = open(logfile, oflags, 0666);
>>                }
>>
>>                if (logfd < 0)
>>                        return error("Unable to append to %s: %s",
>> -                                    *log_file, strerror(errno));
>> +                                    logfile, strerror(errno));
>>        }
>>
>> -       adjust_shared_perm(*log_file);
>> +       adjust_shared_perm(logfile);
>>        close(logfd);
>>        return 0;
>>  }
>> @@ -1309,14 +1307,14 @@ static int log_ref_write(const char *ref_name,
>> const unsigned char *old_sha1,
>>        int logfd, result, written, oflags = O_APPEND | O_WRONLY;
>>        unsigned maxlen, len;
>>        int msglen;
>> -       char *log_file;
>> +       char log_file[PATH_MAX];
>>        char *logrec;
>>        const char *committer;
>>
>>        if (log_all_ref_updates < 0)
>>                log_all_ref_updates = !is_bare_repository();
>>
>> -       result = log_ref_setup(ref_name, &log_file);
>> +       result = log_ref_setup(ref_name, log_file, sizeof(log_file));
>>        if (result)
>>                return result;
>>
>> diff --git a/refs.h b/refs.h
>> index 594c9d9..762ce50 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -69,7 +69,7 @@ extern void unlock_ref(struct ref_lock *lock);
>>  extern int write_ref_sha1(struct ref_lock *lock, const unsigned char
>> *sha1, const char *msg);
>>
>>  /** Setup reflog before using. **/
>> -int log_ref_setup(const char *ref_name, char **log_file);
>> +int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
>>
>>  /** Reads log for the value of ref during at_time. **/
>>  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);
>> --
>> 1.7.1.553.ga798e
>>
>
> I can't get your point.
>
> I don't see any improvement here.  Unless you want to get rid of using
> references on calling functions which is only going to add another buffer to
> the stack, sized PATH_MAX, once that log_file is going to be really
> allocated in the heap after git_snpath().  As folks use to say here:
> "changing six by half a dozen".
>
> Even though I think you will need to turn static log_file or to call
> git_snpath again in the calling functions for this proposed change, don't
> you?
>
>> Causes t5516 to fail, but only if I run it under valgrind.  (Ævar
>> managed to trigger it in other ways apparently.)
>
> I haven't ever seen this happening so I think you have found some
> particularity of valgrind which could route a patch to it.

Actually I think my test failure is related to da3efdb17b, see the
"[PATCH v2 2/2] receive-pack: detect aliased updates which can occur
with symrefs" thread.

The patch under discussion here might be good, I don't have the
knowledge to review it. But it's probably not what's causing the issue
i had with t5516.

[PATCH] check_aliased_update: strcpy() instead of strcat() to copy

[Thomas Rast]

da3efdb (receive-pack: detect aliased updates which can occur with
symrefs, 2010-04-19) introduced two strcat() into uninitialized
strings.  The intent was clearly make a copy of the static buffer used
by find_unique_abbrev(), so use strcpy() instead.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

> Actually I think my test failure is related to da3efdb17b, see the
> "[PATCH v2 2/2] receive-pack: detect aliased updates which can occur
> with symrefs" thread.

Indeed, there's another bug in this one.  (And valgrind catches it
too...  if only I had the patience to let it churn through t5516!)

Unlike the other bug, this one is already in master.

 builtin/receive-pack.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bb34757..7e4129d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -515,9 +515,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
 	dst_cmd->skip_update = 1;
 
 	strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));
-	strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
+	strcpy(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
 	strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));
-	strcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
+	strcpy(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
 	rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
 		 " its target '%s' (%s..%s)",
 		 cmd->ref_name, cmd_oldh, cmd_newh,
-- 
1.7.1.561.g94582

Re: [PATCH] check_aliased_update: strcpy() instead of strcat() to copy

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 10, 2010 at 18:43, Thomas Rast <trast@student.ethz.ch> wrote:
> da3efdb (receive-pack: detect aliased updates which can occur with
> symrefs, 2010-04-19) introduced two strcat() into uninitialized
> strings.  The intent was clearly make a copy of the static buffer used
> by find_unique_abbrev(), so use strcpy() instead.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Tested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

This fixes the problem I was having. Thanks.

Re: [PATCH] check_aliased_update: strcpy() instead of strcat() to copy

[Jay Soffian]

On Thu, Jun 10, 2010 at 2:43 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> da3efdb (receive-pack: detect aliased updates which can occur with
> symrefs, 2010-04-19) introduced two strcat() into uninitialized
> strings.  The intent was clearly make a copy of the static buffer used
> by find_unique_abbrev(), so use strcpy() instead.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
>> Actually I think my test failure is related to da3efdb17b, see the
>> "[PATCH v2 2/2] receive-pack: detect aliased updates which can occur
>> with symrefs" thread.
>
> Indeed, there's another bug in this one.  (And valgrind catches it
> too...  if only I had the patience to let it churn through t5516!)
>
> Unlike the other bug, this one is already in master.
>
>  builtin/receive-pack.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index bb34757..7e4129d 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -515,9 +515,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
>        dst_cmd->skip_update = 1;
>
>        strcpy(cmd_oldh, find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV));
> -       strcat(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
> +       strcpy(cmd_newh, find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV));
>        strcpy(dst_oldh, find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV));
> -       strcat(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
> +       strcpy(dst_newh, find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
>        rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
>                 " its target '%s' (%s..%s)",
>                 cmd->ref_name, cmd_oldh, cmd_newh,

Thanks. I cannot imagine what I was thinking. Maybe a cut-and-paste
error from somewhere else. I am sad this made it all the way to
master.

j.

Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array

[Erick Mattos]

Hi,

We are becoming a little theoretical here so people please be
condescending to us if the chat gets a little boring.  ;-)

2010/6/10 Thomas Rast <trast@student.ethz.ch>:
> What the - side of the hunk above does is returning a local (stack
> allocated) variable, in the form of a pointer to logfile.  Once those
> go out of scope, you have zero guarantees on what happens with them.

Not really.

What the actual log_ref_setup() does when is instantiated is to create
a pointer in the stack, called log_file, to a pointer to a char array.
 This pointer receives the address of a char array of the calling
function because that is why passing by reference is made to.  See
that the calling functions is using the "&" when making the call (If I
was using C++ I would pass by reference the array itself but in C I
can only pass pointer variables by reference that is why the pointer
to a pointer).

Then git_snpath() creates a char array in the heap with the right
content and changes the stack pointer logfile to it.  Then when we do
*log_file = logfile what is happening is that the content of the
pointer in the stack, log_file, which its content is the calling
function char array pointer, is being set to the address of the buffer
created in the heap by git_snpath().  After it, what you have is just
the natural cleanup of the stack variables of the called function but
the calling variable keeps the address of the char array in the heap
created by git_snpath().

> Try the following snippet, it should cause a similar problem:
>
>  #include <stdio.h>
>
>  int* f()
>  {
>        int i;
>        i = 42;
>        return &i;
>  }
>
>  int main()
>  {
>        int *p = f();
>        if (1) {
>                char buf[1024];
>                memset(buf, 0, sizeof(buf));
>        }
>        printf("I got: %d\n", *p);
>  }
>
> Only in this case the issue is so obvious that the compiler will warn
> (at least mine does).

That is another thing.

>> I haven't ever seen this happening so I think you have found some
>> particularity of valgrind which could route a patch to it.
>
> Admittedly my experience is somewhat limited since I don't do C coding
> outside of git and some teaching.  But so far I have not had a single
> false alarm with valgrind (when compiled without optimizations;
> otherwise the compiler may do some magic).

I don't think I am good enough too.  I have always things to learn.
And I hopefully will be always learning new things!  I am addicted to
it.  :-S

Everybody is here to help each other and NOBODY does not commit
mistakes so your help is very welcome but don't deposit all your
beliefs in any piece of software because all of them could contain
mistakes too, even if the best ever existent programmers had made them
in whole.

Programming is teaching a limited dumb equipment to be clever.  So it
is in itself a contradiction.

Best regards

Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array

[Jeff King]

On Thu, Jun 10, 2010 at 08:09:36PM -0300, Erick Mattos wrote:

> 2010/6/10 Thomas Rast <trast@student.ethz.ch>:
> > What the - side of the hunk above does is returning a local (stack
> > allocated) variable, in the form of a pointer to logfile.  Once those
> > go out of scope, you have zero guarantees on what happens with them.
> 
> Not really.
> 
> What the actual log_ref_setup() does when is instantiated is to create
> a pointer in the stack, called log_file, to a pointer to a char array.
>  This pointer receives the address of a char array of the calling
> function because that is why passing by reference is made to.  See
> that the calling functions is using the "&" when making the call (If I
> was using C++ I would pass by reference the array itself but in C I
> can only pass pointer variables by reference that is why the pointer
> to a pointer).

No, Thomas is right. This invokes undefined behavior. We point the
passed-in log_file pointer to the front of a character array with
automatic duration. After log_ref_setup returns, we must never
dereference that pointer again, but we do. So we need this patch or
something like it.

In practice, it worked because allocating on the stack is really just
about bumping the stack pointer, so that memory sits there until another
function call needs it for stack variables. After returning from
log_ref_setup, we don't actually make any other function calls before
calling open(log_file), so the buffer was still there, untouched. There
is a later use of log_file which is probably bogus, but was likely never
triggered because it is in an unlikely error conditional.

> Then git_snpath() creates a char array in the heap with the right
> content and changes the stack pointer logfile to it.  Then when we do

No, it doesn't. git_snpath writes into the buffer you provide it, just
like snprintf (hence the name).

> > Admittedly my experience is somewhat limited since I don't do C coding
> > outside of git and some teaching.  But so far I have not had a single
> > false alarm with valgrind (when compiled without optimizations;
> > otherwise the compiler may do some magic).

We have some false positives in git, but you don't see them because
t/valgrind/default.supp suppresses them. For example:

  http://thread.gmane.org/gmane.comp.version-control.git/106335/focus=107302

If you are using a binary package of valgrind, it probably ships with
some system-specific suppressions, too. Right now valgrind on Debian
unstable is next to useless because glibc has been upgraded to 2.11, but
the suppressions haven't been updated. So you get false positives all
over the place because of clever architecture-specific optimizations
(e.g., I am seeing a lot of __strlen_sse2 problems, which are probably
just the function over-reading its input data because processing big
chunks is faster).

-Peff

Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array

[Erick Mattos]

Hi,

2010/6/11 Jeff King <peff@peff.net>:
> No, Thomas is right. This invokes undefined behavior. We point the
> passed-in log_file pointer to the front of a character array with
> automatic duration. After log_ref_setup returns, we must never
> dereference that pointer again, but we do. So we need this patch or
> something like it.

You and Thomas are right on this subject.  I don't know when and how I
could see a malloc() in git_(v)snpath().  My fault.

>> Then git_snpath() creates a char array in the heap with the right
>> content and changes the stack pointer logfile to it.  Then when we do
>
> No, it doesn't. git_snpath writes into the buffer you provide it, just
> like snprintf (hence the name).

The source of my failure.

> We have some false positives in git, but you don't see them because
> t/valgrind/default.supp suppresses them. For example:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/106335/focus=107302
>
> If you are using a binary package of valgrind, it probably ships with
> some system-specific suppressions, too. Right now valgrind on Debian
> unstable is next to useless because glibc has been upgraded to 2.11, but
> the suppressions haven't been updated. So you get false positives all
> over the place because of clever architecture-specific optimizations
> (e.g., I am seeing a lot of __strlen_sse2 problems, which are probably
> just the function over-reading its input data because processing big
> chunks is faster).
>
> -Peff

Thanks for the extended explanation about valgrind.

Regards to all