Re: [PATCH] system_path: use a static buffer
On mié, 2011-03-16 at 16:58 +0100, Erik Faye-Lund wrote:
Show 33 quoted lines
> 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.
Show 11 quoted lines
>
> > @@ -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?
Carlos Martín Nieto <cmn@elego.de>