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

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 <redacted>
Reported-by: Ævar Arnfjörð Bjarmason <redacted>
---

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

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 <redacted>
Reported-by: Ævar Arnfjörð Bjarmason <redacted>
---

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

Hi there,

2010/6/10 Thomas Rast [off-list ref]

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

Erick Mattos wrote:

2010/6/10 Thomas Rast [off-list ref]

quoted

-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

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 [off-list ref]:

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.

quoted

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

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

2010/6/10 Thomas Rast [off-list ref]:

quoted

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).

quoted

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

Hi,

2010/6/11 Jeff King [off-list ref]:

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.

quoted

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