Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself

["Carlos Martín Nieto" <cmn@elego.de>]

On lun, 2011-03-14 at 13:25 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > Sometimes (at least in t-0001-init.sh test 12), the return value of
> > make_absolute_path() is passed to it as an argument, making the first
> 
> I don't think it is a bad idea per-se to avoid a copy from the same memory
> location into the same memory location, but independent of the necessity
> of fixes at the low-level, shouldn't we fix the callers that do not check
> if what they have is already absolute?

 If we'd like the semantics to be "whatever I had, I now know what the
absolute path is" then we could make the check in the beginning of the
function, to centralise the check. If the semantics should be "I don't
have an absolute path, so I need to figure out what it is", then there
should be a check before calling make_absolute_path() (the name suggests
the second).

 As there are only ~15-20 uses of make_absolute_path(), we could just
leave this as it is (as it works under normal conditions and causes
valgrind to warn us), and I'll examine the callers tomorrow.

 There is however the extra functionality the function offers, namely
resolving links. It might be good to split it into two functions so each
caller can specify what it wants.

 There is also this in setup.c:prefix_path()

        const char *orig = path;
        char *sanitized;
        if (is_absolute_path(orig)) {
                const char *temp = make_absolute_path(path);
                sanitized = xmalloc(len + strlen(temp) + 1);
                strcpy(sanitized, temp);
        } else {
                sanitized = xmalloc(len + strlen(path) + 1);
                if (len)
                        memcpy(sanitized, prefix, len);
                strcpy(sanitized + len, path);
        }

which doesn't seem to make sense. Should the call to
make_absolute_path() actually be a call to a function resolve_links()
that just incorporates that functionality from make_absolute_path()?


> 
> > and second arguments to strlcpy() the same, making the test fail when
> > run under valgrind.
> >
> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> > ---
> >
> > This patch assumes the path returned by make_absolute_path() is never
> > longer than PATH_MAX, which I think is a safe assumption.
> 
> >
> >  abspath.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/abspath.c b/abspath.c
> > index 91ca00f..9149a98 100644
> > --- a/abspath.c
> > +++ b/abspath.c
> > @@ -24,7 +24,7 @@ const char *make_absolute_path(const char *path)
> >  	char *last_elem = NULL;
> >  	struct stat st;
> >  
> > -	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> > +	if (buf != path && strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> >  		die ("Too long path: %.*s", 60, path);
> >  
> >  	while (depth--) {