[PATCH] system_path: use a static buffer

[PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

Make system_path behave like the other path functions by using a
static buffer, fixing a memory leak.

Also make sure the prefix pointer is always initialized to either
PREFIX or NULL.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

This fixes the leak I was trying to fix with my original patch, but
this seems much cleaner

 exec_cmd.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 38545e8..12ce017 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -9,11 +9,11 @@ static const char *argv0_path;
 const char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
-	static const char *prefix;
+	static const char *prefix = NULL;
 #else
 	static const char *prefix = PREFIX;
 #endif
-	struct strbuf d = STRBUF_INIT;
+	static char buf[PATH_MAX+1];
 
 	if (is_absolute_path(path))
 		return path;
@@ -33,9 +33,8 @@ const char *system_path(const char *path)
 	}
 #endif
 
-	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	snprintf(buf, PATH_MAX, "%s/%s", prefix, path);
+	return buf;
 }
 
 const char *git_extract_argv0_path(const char *argv0)
-- 
1.7.4.1

Re: [PATCH] system_path: use a static buffer

[Erik Faye-Lund]

On Wed, Mar 16, 2011 at 12:26 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
> Make system_path behave like the other path functions by using a
> static buffer, fixing a memory leak.
>
> Also make sure the prefix pointer is always initialized to either
> PREFIX or NULL.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>
> This fixes the leak I was trying to fix with my original patch, but
> this seems much cleaner
>
>  exec_cmd.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 38545e8..12ce017 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -9,11 +9,11 @@ static const char *argv0_path;
>  const char *system_path(const char *path)
>  {
>  #ifdef RUNTIME_PREFIX
> -       static const char *prefix;
> +       static const char *prefix = NULL;
>  #else
>        static const char *prefix = PREFIX;
>  #endif
> -       struct strbuf d = STRBUF_INIT;
> +       static char buf[PATH_MAX+1];

Why PATH_MAX + 1? POSIX says that PATH_MAX includes the null-termination...

> @@ -33,9 +33,8 @@ const char *system_path(const char *path)
>        }
>  #endif
>
> -       strbuf_addf(&d, "%s/%s", prefix, path);
> -       path = strbuf_detach(&d, NULL);
> -       return path;
> +       snprintf(buf, PATH_MAX, "%s/%s", prefix, path);

Perhaps "snprintf(buf, sizeof(buf) - 1, "%s/%s", prefix, path);" instead?

Re: [PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

On mié, 2011-03-16 at 16:58 +0100, Erik Faye-Lund wrote:
> On Wed, Mar 16, 2011 at 12:26 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
> > Make system_path behave like the other path functions by using a
> > static buffer, fixing a memory leak.
> >
> > Also make sure the prefix pointer is always initialized to either
> > PREFIX or NULL.
> >
> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> > ---
> >
> > This fixes the leak I was trying to fix with my original patch, but
> > this seems much cleaner
> >
> >  exec_cmd.c |    9 ++++-----
> >  1 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 38545e8..12ce017 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -9,11 +9,11 @@ static const char *argv0_path;
> >  const char *system_path(const char *path)
> >  {
> >  #ifdef RUNTIME_PREFIX
> > -       static const char *prefix;
> > +       static const char *prefix = NULL;
> >  #else
> >        static const char *prefix = PREFIX;
> >  #endif
> > -       struct strbuf d = STRBUF_INIT;
> > +       static char buf[PATH_MAX+1];
> 
> Why PATH_MAX + 1? POSIX says that PATH_MAX includes the null-termination...

 A lot of other code I've been looking at uses it. I was not aware it
included the terminator.

> 
> > @@ -33,9 +33,8 @@ const char *system_path(const char *path)
> >        }
> >  #endif
> >
> > -       strbuf_addf(&d, "%s/%s", prefix, path);
> > -       path = strbuf_detach(&d, NULL);
> > -       return path;
> > +       snprintf(buf, PATH_MAX, "%s/%s", prefix, path);
> 
> Perhaps "snprintf(buf, sizeof(buf) - 1, "%s/%s", prefix, path);" instead?

 The manpage states that the size parameter includes the
null-terminator, so sizeof(buf) would be better, I think. I'll send out
a new mail with the updated patch.

 Shouldn't we check to see if there was truncation (i.e. return value >=
sizeof(buf)) and die in that case?

[PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

Make system_path behave like the other path functions by using a
static buffer, fixing a memory leak.

Also make sure the prefix pointer is always initialized to either
PREFIX or NULL.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 exec_cmd.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 38545e8..5686952 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -9,11 +9,11 @@ static const char *argv0_path;
 const char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
-	static const char *prefix;
+	static const char *prefix = NULL;
 #else
 	static const char *prefix = PREFIX;
 #endif
-	struct strbuf d = STRBUF_INIT;
+	static char buf[PATH_MAX];
 
 	if (is_absolute_path(path))
 		return path;
@@ -33,9 +33,10 @@ const char *system_path(const char *path)
 	}
 #endif
 
-	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	if (snprintf(buf, sizeof(buf), "%s/%s", prefix, path) >= sizeof(buf))
+		die("system path too long for %s", path);
+
+	return buf;
 }
 
 const char *git_extract_argv0_path(const char *argv0)
-- 
1.7.4.1

Re: [PATCH] system_path: use a static buffer

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> Make system_path behave like the other path functions by using a
> static buffer, fixing a memory leak.
>
> Also make sure the prefix pointer is always initialized to either
> PREFIX or NULL.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---

Have you made sure all the callers are Ok with this change?

If somebody called system_path(GIT_EXEC_PATH), saved the result in a
variable without copying, and then called system_path(ETC_GITATTRIBUTES),
his variable may now have a value unrelated to GIT_EXEC_PATH, and you
would fix all such callers to save the value away with strdup().

[PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

Make system_path behave like the other path functions by using a
static buffer, fixing a memory leak.

Also make sure the prefix pointer is always initialized to either
PREFIX or NULL.

git_etc_gitattributes and git_etc_gitconfig are the only users who are
affected by this change. Make them use a static buffer, which fits
their use better as well.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

On mié, 2011-03-16 at 13:43 -0700, Junio C Hamano wrote:
Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > Make system_path behave like the other path functions by using a
> > static buffer, fixing a memory leak.
> >
> > Also make sure the prefix pointer is always initialized to either
> > PREFIX or NULL.
> >
> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> > ---
> 
> Have you made sure all the callers are Ok with this change?
> 
> If somebody called system_path(GIT_EXEC_PATH), saved the result in a
> variable without copying, and then called system_path(ETC_GITATTRIBUTES),
> his variable may now have a value unrelated to GIT_EXEC_PATH, and you
> would fix all such callers to save the value away with strdup().


I checked again, and except for the ones changed in this patch, the
rest copy it to their own buffer or pass it to puts, setenv or
strbuf_addstr.

The way these functions are used suggest the caller expects them to
deal with their own memory, so that's what I've done.

TBH, valgrind only reports a win of ~6-7kB when doing git log on
git.git, but it's a step in the right direction (and adds consistency
to system_path, which is the main win).

 attr.c     |    6 +++---
 config.c   |    6 +++---
 exec_cmd.c |   11 ++++++-----
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/attr.c b/attr.c
index 6aff695..64d803f 100644
--- a/attr.c
+++ b/attr.c
@@ -467,9 +467,9 @@ static void drop_attr_stack(void)
 
 const char *git_etc_gitattributes(void)
 {
-	static const char *system_wide;
-	if (!system_wide)
-		system_wide = system_path(ETC_GITATTRIBUTES);
+	static char system_wide[PATH_MAX];
+	if (!system_wide[0])
+		strlcpy(system_wide, system_path(ETC_GITATTRIBUTES), PATH_MAX);
 	return system_wide;
 }
 
diff --git a/config.c b/config.c
index 822ef83..cd1c295 100644
--- a/config.c
+++ b/config.c
@@ -808,9 +808,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 
 const char *git_etc_gitconfig(void)
 {
-	static const char *system_wide;
-	if (!system_wide)
-		system_wide = system_path(ETC_GITCONFIG);
+	static char system_wide[PATH_MAX];
+	if (!system_wide[0])
+		strlcpy(system_wide, system_path(ETC_GITCONFIG), PATH_MAX);
 	return system_wide;
 }
 
diff --git a/exec_cmd.c b/exec_cmd.c
index 38545e8..5686952 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -9,11 +9,11 @@ static const char *argv0_path;
 const char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
-	static const char *prefix;
+	static const char *prefix = NULL;
 #else
 	static const char *prefix = PREFIX;
 #endif
-	struct strbuf d = STRBUF_INIT;
+	static char buf[PATH_MAX];
 
 	if (is_absolute_path(path))
 		return path;
@@ -33,9 +33,10 @@ const char *system_path(const char *path)
 	}
 #endif
 
-	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	if (snprintf(buf, sizeof(buf), "%s/%s", prefix, path) >= sizeof(buf))
+		die("system path too long for %s", path);
+
+	return buf;
 }
 
 const char *git_extract_argv0_path(const char *argv0)
-- 
1.7.4.1

[PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

Make system_path behave like the other path functions by using a
static buffer, fixing a memory leak.

Also make sure the prefix pointer is always initialized to either
PREFIX or NULL.

git_etc_gitattributes and git_etc_gitconfig are the only users who are
affected by this change. Make them use a static buffer, which fits
their use better as well.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

It was pointed out that one should always check for an encoding error
(-1) with the printf family. I'm not sure how likely this is to
happen, but this should make the code extra portable :)

 attr.c     |    6 +++---
 config.c   |    6 +++---
 exec_cmd.c |   15 ++++++++++-----
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/attr.c b/attr.c
index 6aff695..64d803f 100644
--- a/attr.c
+++ b/attr.c
@@ -467,9 +467,9 @@ static void drop_attr_stack(void)
 
 const char *git_etc_gitattributes(void)
 {
-	static const char *system_wide;
-	if (!system_wide)
-		system_wide = system_path(ETC_GITATTRIBUTES);
+	static char system_wide[PATH_MAX];
+	if (!system_wide[0])
+		strlcpy(system_wide, system_path(ETC_GITATTRIBUTES), PATH_MAX);
 	return system_wide;
 }
 
diff --git a/config.c b/config.c
index 822ef83..cd1c295 100644
--- a/config.c
+++ b/config.c
@@ -808,9 +808,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 
 const char *git_etc_gitconfig(void)
 {
-	static const char *system_wide;
-	if (!system_wide)
-		system_wide = system_path(ETC_GITCONFIG);
+	static char system_wide[PATH_MAX];
+	if (!system_wide[0])
+		strlcpy(system_wide, system_path(ETC_GITCONFIG), PATH_MAX);
 	return system_wide;
 }
 
diff --git a/exec_cmd.c b/exec_cmd.c
index 38545e8..35d5cd8 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -9,11 +9,12 @@ static const char *argv0_path;
 const char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
-	static const char *prefix;
+	static const char *prefix = NULL;
 #else
 	static const char *prefix = PREFIX;
 #endif
-	struct strbuf d = STRBUF_INIT;
+	static char buf[PATH_MAX];
+	int ret;
 
 	if (is_absolute_path(path))
 		return path;
@@ -33,9 +34,13 @@ const char *system_path(const char *path)
 	}
 #endif
 
-	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
+	if (ret >= sizeof(buf))
+		die("system path too long for %s", path);
+	else if (ret < 0)
+		die_errno("encoding error");
+
+	return buf;
 }
 
 const char *git_extract_argv0_path(const char *argv0)
-- 
1.7.4.1

Re: [PATCH] system_path: use a static buffer

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> +	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
> +	if (ret >= sizeof(buf))
> +		die("system path too long for %s", path);
> +	else if (ret < 0)
> +		die_errno("encoding error");

POSIX says snprintf() should set errno in this case, and your use of
die_errno() would show that information, but what is "encoding error"?

Just being curious, as I suspect that "snprintf() returned an error" may
be more appropriate, if the answer is "I don't know what kind of error it
is, but snprintf() found something faulty while encoding so I chose to
call it encoding error".

By the way, thanks for checking all the callers.

Re: [PATCH] system_path: use a static buffer

[Nguyen Thai Ngoc Duy]

On Thu, Mar 17, 2011 at 9:24 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
> -       static const char *system_wide;
> -       if (!system_wide)
> -               system_wide = system_path(ETC_GITATTRIBUTES);
> +       static char system_wide[PATH_MAX];

...

> -       static const char *system_wide;
> -       if (!system_wide)
> -               system_wide = system_path(ETC_GITCONFIG);
> +       static char system_wide[PATH_MAX];

...

>  #ifdef RUNTIME_PREFIX
> -       static const char *prefix;
> +       static const char *prefix = NULL;
>  #else
>        static const char *prefix = PREFIX;
>  #endif
> -       struct strbuf d = STRBUF_INIT;
> +       static char buf[PATH_MAX];
> +       int ret;

It was pointed out elsewhere [1] that PATH_MAX only specifies max
length of a path element, not full path. I think we'd need to stay
away from preallocated PATH_MAX-sized arrays.

[1] http://mid.gmane.org/AANLkTikXvx7-Q8B_dqG5mMHGK_Rw-dFaeQdXi0zW98SD@mail.gmail.com
-- 
Duy

Re: [PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

On vie, 2011-03-18 at 00:25 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > +	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
> > +	if (ret >= sizeof(buf))
> > +		die("system path too long for %s", path);
> > +	else if (ret < 0)
> > +		die_errno("encoding error");
> 
> POSIX says snprintf() should set errno in this case, and your use of
> die_errno() would show that information, but what is "encoding error"?
> 
> Just being curious, as I suspect that "snprintf() returned an error" may
> be more appropriate, if the answer is "I don't know what kind of error it
> is, but snprintf() found something faulty while encoding so I chose to
> call it encoding error".


 My manpage says snprintf returns -1 if there was an output or encoding
error. As there couldn't be an output error because it's writing to
memory and we can't output what snprintf chocked on because whatever
die_errno uses will also choke on it, I just put "encoding error". I'd
put "error assembling system path" as the actual error message, I guess.

   cmn

Re: [PATCH] system_path: use a static buffer

[Jeff King]

On Mon, Mar 21, 2011 at 10:56:19AM +0100, Carlos Martín Nieto wrote:

> On vie, 2011-03-18 at 00:25 -0700, Junio C Hamano wrote:
> > Carlos Martín Nieto <cmn@elego.de> writes:
> > 
> > > +	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
> > > +	if (ret >= sizeof(buf))
> > > +		die("system path too long for %s", path);
> > > +	else if (ret < 0)
> > > +		die_errno("encoding error");
> > 
> > POSIX says snprintf() should set errno in this case, and your use of
> > die_errno() would show that information, but what is "encoding error"?
> > 
> > Just being curious, as I suspect that "snprintf() returned an error" may
> > be more appropriate, if the answer is "I don't know what kind of error it
> > is, but snprintf() found something faulty while encoding so I chose to
> > call it encoding error".
> 
>  My manpage says snprintf returns -1 if there was an output or encoding
> error. As there couldn't be an output error because it's writing to
> memory and we can't output what snprintf chocked on because whatever
> die_errno uses will also choke on it, I just put "encoding error". I'd
> put "error assembling system path" as the actual error message, I guess.

FWIW, we don't catch snprintf failures in 99% of the calls in git. Most
calls just ignore the return value, and some even directly use the
return value to add to a length. The one place that actually does check
for the error is strbuf_vaddf, which just says "your vsnprintf is
broken" and dies.

So I'm not sure how much we really care about this error code path. If
anything, we should be replacing all of the calls with something like:

  static const char buggy_sprintf_msg[] =
  "BUG: vsnprintf returned %d; either we fed it a bogus format string\n"
  "(our bug) or your libc is buggy and returns an error when it should\n"
  "tell us how much space is needed. The format string was:\n"
  "%s\n";
  int xsnprintf(char *out, size_t size, const char *fmt, ...)
  {
          va_list ap;
          int r;

          va_start(ap, fmt);
          r = vsnprintf(out, size, fmt, ap);
          va_end(ap);

          if (r < 0)
                  die(buggy_sprintf_msg, r, fmt);
          return r;
  }

-Peff

Re: [PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

On lun, 2011-03-21 at 07:14 -0400, Jeff King wrote:
> On Mon, Mar 21, 2011 at 10:56:19AM +0100, Carlos Martín Nieto wrote:
> 
> > On vie, 2011-03-18 at 00:25 -0700, Junio C Hamano wrote:
> > > Carlos Martín Nieto <cmn@elego.de> writes:
> > > 
> > > > +	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
> > > > +	if (ret >= sizeof(buf))
> > > > +		die("system path too long for %s", path);
> > > > +	else if (ret < 0)
> > > > +		die_errno("encoding error");
> > > 
> > > POSIX says snprintf() should set errno in this case, and your use of
> > > die_errno() would show that information, but what is "encoding error"?
> > > 
> > > Just being curious, as I suspect that "snprintf() returned an error" may
> > > be more appropriate, if the answer is "I don't know what kind of error it
> > > is, but snprintf() found something faulty while encoding so I chose to
> > > call it encoding error".
> > 
> >  My manpage says snprintf returns -1 if there was an output or encoding
> > error. As there couldn't be an output error because it's writing to
> > memory and we can't output what snprintf chocked on because whatever
> > die_errno uses will also choke on it, I just put "encoding error". I'd
> > put "error assembling system path" as the actual error message, I guess.
> 
> FWIW, we don't catch snprintf failures in 99% of the calls in git. Most
> calls just ignore the return value, and some even directly use the
> return value to add to a length. The one place that actually does check
> for the error is strbuf_vaddf, which just says "your vsnprintf is
> broken" and dies.

 It's not actually likely we'll ever meet this error if the only one
allowed to set the format string is the programmer (and to do otherwise
is a security risk).

> 
> So I'm not sure how much we really care about this error code path. If
> anything, we should be replacing all of the calls with something like:
> 
>   static const char buggy_sprintf_msg[] =
>   "BUG: vsnprintf returned %d; either we fed it a bogus format string\n"
>   "(our bug) or your libc is buggy and returns an error when it should\n"
>   "tell us how much space is needed. The format string was:\n"
>   "%s\n";
>   int xsnprintf(char *out, size_t size, const char *fmt, ...)
>   {
>           va_list ap;
>           int r;
> 
>           va_start(ap, fmt);
>           r = vsnprintf(out, size, fmt, ap);
>           va_end(ap);
> 
>           if (r < 0)
>                   die(buggy_sprintf_msg, r, fmt);
>           return r;
>   }

 Or we could overload (#define) snprintf and replace it with the
paranoid. It'd go nicely with the vsnprintf that tries to work around
the Windows implementation.

 I don't feel that strongly we should have the extra check there, seeing
how it's rare and not checked anywhere else.

   cmn

Re: [PATCH] system_path: use a static buffer

[Jeff King]

On Mon, Mar 21, 2011 at 04:26:29PM +0100, Carlos Martín Nieto wrote:

> > FWIW, we don't catch snprintf failures in 99% of the calls in git. Most
> > calls just ignore the return value, and some even directly use the
> > return value to add to a length. The one place that actually does check
> > for the error is strbuf_vaddf, which just says "your vsnprintf is
> > broken" and dies.
> 
>  It's not actually likely we'll ever meet this error if the only one
> allowed to set the format string is the programmer (and to do otherwise
> is a security risk).

Agreed.

>  Or we could overload (#define) snprintf and replace it with the
> paranoid. It'd go nicely with the vsnprintf that tries to work around
> the Windows implementation.
> 
>  I don't feel that strongly we should have the extra check there, seeing
> how it's rare and not checked anywhere else.

Yeah, I am happy to just drop it. AFAICT, an error return from snprintf
is a bug in your program[1] or a bug in libc. In either case, it should
fail or produce bogus output on the first run, and we don't need to
worry about checking errors all the time. Noting the error helps a
little with detection, but since we already install our own vsnprintf on
known-buggy platforms, I haven't seen a single complaint.

-Peff

[1] The only error I managed to get out of eglibc was trying to format
"%" (i.e., percent followed by NUL). Skimming the eglibc code (gaah, my
eyes!) it looks like under some conditions it can allocate small work
buffers, and return an error if malloc fails. So yeah, I guess it can
fail randomly, but it seems pretty unlikely.

Re: [PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

On lun, 2011-03-21 at 11:51 -0400, Jeff King wrote:
> Yeah, I am happy to just drop it. AFAICT, an error return from snprintf
> is a bug in your program[1] or a bug in libc. In either case, it should
> fail or produce bogus output on the first run, and we don't need to
> worry about checking errors all the time. Noting the error helps a
> little with detection, but since we already install our own vsnprintf on
> known-buggy platforms, I haven't seen a single complaint.

 Then the patch in <1300359664-6230-1-git-send-email-cmn@elego.de>
should do the trick.

   cmn

[PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

Make system_path behave like the other path functions by using a
static buffer, plugging a memory leak.

Also make sure the prefix pointer is always initialized to either
PREFIX or NULL.

git_etc_gitattributes and git_etc_gitconfig are the only users who are
affected by this change. Make them use a static buffer, which fits
their use better as well.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

 Slightly changed version of the patch in pu with the error strings
changed. Junio, do you want this patch at all. I think in your "what's
cooking" messages, you said you don't like this type of band-aid? As
it's not that big a deal, I'll drop it if you don't want it.

 attr.c     |    6 +++---
 config.c   |    6 +++---
 exec_cmd.c |   15 ++++++++++-----
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/attr.c b/attr.c
index 6aff695..64d803f 100644
--- a/attr.c
+++ b/attr.c
@@ -467,9 +467,9 @@ static void drop_attr_stack(void)
 
 const char *git_etc_gitattributes(void)
 {
-	static const char *system_wide;
-	if (!system_wide)
-		system_wide = system_path(ETC_GITATTRIBUTES);
+	static char system_wide[PATH_MAX];
+	if (!system_wide[0])
+		strlcpy(system_wide, system_path(ETC_GITATTRIBUTES), PATH_MAX);
 	return system_wide;
 }
 
diff --git a/config.c b/config.c
index 822ef83..cd1c295 100644
--- a/config.c
+++ b/config.c
@@ -808,9 +808,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 
 const char *git_etc_gitconfig(void)
 {
-	static const char *system_wide;
-	if (!system_wide)
-		system_wide = system_path(ETC_GITCONFIG);
+	static char system_wide[PATH_MAX];
+	if (!system_wide[0])
+		strlcpy(system_wide, system_path(ETC_GITCONFIG), PATH_MAX);
 	return system_wide;
 }
 
diff --git a/exec_cmd.c b/exec_cmd.c
index 38545e8..8d0fa49 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -9,11 +9,12 @@ static const char *argv0_path;
 const char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
-	static const char *prefix;
+	static const char *prefix = NULL;
 #else
 	static const char *prefix = PREFIX;
 #endif
-	struct strbuf d = STRBUF_INIT;
+	static char buf[PATH_MAX];
+	int ret;
 
 	if (is_absolute_path(path))
 		return path;
@@ -33,9 +34,13 @@ const char *system_path(const char *path)
 	}
 #endif
 
-	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
+	if (ret >= sizeof(buf))
+		die("system path too long");
+	else if (ret < 0)
+		die_errno("snprintf reported an error");
+
+	return buf;
 }
 
 const char *git_extract_argv0_path(const char *argv0)
-- 
1.7.4.1

Re: [PATCH] system_path: use a static buffer

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

>  Slightly changed version of the patch in pu with the error strings
> changed. Junio, do you want this patch at all. I think in your "what's
> cooking" messages, you said you don't like this type of band-aid? As
> it's not that big a deal, I'll drop it if you don't want it.

The "band-aid" refers to the topic listed after your topic, I think.  I
didn't even have a comment on your topic in the message.

We'll be in pre-release freeze for 1.7.5 soon, and I want to see patches
that deal with internal clean-up held for now and resubmit after 1.7.5 is
released.  This cycle lasted a bit too long but hopefully we can conclude
it by the mid April.

Thanks.

Re: [PATCH] system_path: use a static buffer

[Carlos Martín Nieto]

On Thu, 2011-03-31 at 15:42 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> >  Slightly changed version of the patch in pu with the error strings
> > changed. Junio, do you want this patch at all. I think in your "what's
> > cooking" messages, you said you don't like this type of band-aid? As
> > it's not that big a deal, I'll drop it if you don't want it.
> 
> The "band-aid" refers to the topic listed after your topic, I think.  I
> didn't even have a comment on your topic in the message.

 OK, I misunderstood.

> 
> We'll be in pre-release freeze for 1.7.5 soon, and I want to see patches
> that deal with internal clean-up held for now and resubmit after 1.7.5 is
> released.  This cycle lasted a bit too long but hopefully we can conclude
> it by the mid April.

 Fair enough. I'll re-send after the release.

   cmn