[PATCH] lib/cmd/bup.c: Accept traditional 4.4BSD realpath

[Greg Troxel]

POSIX requires more than the original implementation, and this code
relied on the POSIX behavior. Rely on the intersection of POSIX and
the traditional 4.4BSD behavior. (realpath(3) originated in 4.4BSD.)

Signed-off-by: Greg Troxel <g...@lexort.com>
---
lib/cmd/bup.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/lib/cmd/bup.c b/lib/cmd/bup.c
index 9cd1f388..43dcacb3 100644
--- a/lib/cmd/bup.c
+++ b/lib/cmd/bup.c
@@ -201,9 +201,18 @@ static char *find_in_path(const char * const name, const char * const path)
return result;
}

+/*
+ * This procedure uses realpath(3). While there is a POSIX
+ * specification that requires accepting a 2nd argument of NULL, the
+ * traditional 4.4BSD implementation (where realpath originated) does
+ * not have this requirement. Therefore, pass in a buffer, and expect
+ * that the return value points to the buffer (and thus, do not free
+ * it).
+ */
static char *find_exe_parent(const char * const argv_0)
{
char *candidate = NULL;
+ char resolvedname[MAXPATHLEN];
const char * const slash = strchr(argv_0, '/');
if (slash) {
candidate = strdup(argv_0);
@@ -215,24 +224,24 @@ static char *find_exe_parent(const char * const argv_0)
argv_0);
char *path_exe = find_in_path(argv_0, env_path);
if (path_exe) {
- char * abs_exe = realpath(path_exe, NULL);
+ char * abs_exe = realpath(path_exe, resolvedname);
if (!abs_exe)
die(2, "cannot resolve path (%s): %s\n",
strerror(errno), path_exe);
- free(path_exe);
- candidate = abs_exe;
+ assert(abs_exe == resolvedname);
+ candidate = strdup(resolvedname);
+ assert(candidate);
}
}
if (!candidate)
return NULL;

- char * const abs_exe = realpath(candidate, NULL);
+ char * const abs_exe = realpath(candidate, resolvedname);
if (!abs_exe)
die(2, "cannot resolve path (%s): %s\n", strerror(errno), candidate);
free(candidate);
char * const abs_parent = strdup(dirname(abs_exe));
assert(abs_parent);
- free(abs_exe);
return abs_parent;
}

--
2.36.1

[Rob Browning]

Greg Troxel <g...@lexort.com> writes:

> POSIX requires more than the original implementation, and this code
> relied on the POSIX behavior. Rely on the intersection of POSIX and
> the traditional 4.4BSD behavior. (realpath(3) originated in 4.4BSD.)

I'll have to double-check, but my recollection is that (in general) this
can be trouble. i.e. the platform's actual limit may be much higher
(than PATH_MAX too), which can lead to other trouble if you're not
careful (later, when combining code).

FWIW, here a (naive) rgrep shows all kinds of potential different values
for MAXPATHLEN in /usr/include (1024, 256, 2048, 4096, PATH_MAX). And
not sure, but I suspect the acual Linux limit may either be much higher,
or fully dynamic.

In any case, I'll have to think about this, but if issue only affects
the really old BSDs (or is it still true for current BSDs?), then I
wonder if we might want to consider making POSIX our baseline here, and
if we think it's worth it, create code that will paper over the static
situation on plaforms that don't support POSIX.

But I haven't thought carefully about it yet -- will plan to.

Thanks
--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[Greg Troxel]

It would be fairly easy to make a bup_realpath() that takes only one
arg, and uses the NULL form normally, except where it's known not to
work, in which case it uses the 2nd and does strdup. I think that would
be satisfactory to all.

[Rob Browning]

OK, I can see what that might look like, unless you'd prefer to.

And I'm wondering about detection. For example, does the older realpath
crash on a NULL argument, or just set errno?

Also, if MAXPATHLEN isn't POSIX, but something else like PATH_MAX is
(not sure), then wonder if we could use that interchangably to make it
more portable.

Hmm, I'll also go see what gnulib does, if anything. Ahh:

https://www.gnu.org/software/gnulib/manual/html_node/realpath.html

and cf. canonicalize_file_name:

https://www.gnu.org/software//gnulib/manual/html_node/Benefits.html

Suspect using those might involve more complexity than we want/need, but
the docs provide some additional info if nothing else. (I did add their
intprops.h a while back for the overflow tests.)

[Greg Troxel]

Rob Browning <r...@defaultvalue.org> writes:

> Greg Troxel <g...@lexort.com> writes:
>
>> It would be fairly easy to make a bup_realpath() that takes only one
>> arg, and uses the NULL form normally, except where it's known not to
>> work, in which case it uses the 2nd and does strdup. I think that would
>> be satisfactory to all.
>
> OK, I can see what that might look like, unless you'd prefer to.

I feel like it's my fault we need it so I was going to, but I don't
prefer to. I think it's just (writing bad C pseudocode too fast!):

#if netbsd and version <= 5
#define OLD_REALPATH
#endif

char *
bup_realpath(const char *pathname)
{
#ifndef OLD_REALPATH
return realpath(pathname, NULL):
#else
char *ret;
char resolvedname[MAXPATHLEN];

ret = realpath(pathname, resolvedname);

if (ret != NULL)
{
assert(ret == resolvedname);
ret = strdup(ret)
}
return ret;
#endif
}

and that's probably ok once fixed.

> And I'm wondering about detection. For example, does the older realpath
> crash on a NULL argument, or just set errno?

It crashes.

> Also, if MAXPATHLEN isn't POSIX, but something else like PATH_MAX is
> (not sure), then wonder if we could use that interchangably to make it
> more portable.

The realpath page in POSIX says MAXPATHLEN.

> Hmm, I'll also go see what gnulib does, if anything. Ahh:
>
> https://www.gnu.org/software/gnulib/manual/html_node/realpath.html
>
> and cf. canonicalize_file_name:
>
> https://www.gnu.org/software//gnulib/manual/html_node/Benefits.html
>
> Suspect using those might involve more complexity than we want/need, but
> the docs provide some additional info if nothing else. (I did add their
> intprops.h a while back for the overflow tests.)

Your pushback against always using MAXPATHLEN is valid; that imposes a
lower limit on results on systems with one value of MAXPATHLEN that
dynamically support a higher value. So I immediately came around to
conditionalizing it, and then realized it was easy.

[Greg Troxel]

I was confused about constants.

POSIX specifies PATH_MAX.
NetBSD man pages, 5 and 9, say MAXPATHLEN

But NetBSD sys/sys/param.h defines MAXPATHLEN to PATH_MAX, so we can
just use PATH_MAX.

[Rob Browning]

Greg Troxel <g...@lexort.com> writes:

> I feel like it's my fault we need it so I was going to, but I don't
> prefer to. I think it's just (writing bad C pseudocode too fast!):
>
> #if netbsd and version <= 5
> #define OLD_REALPATH
> #endif

Maybe things have changed, but it looked like this suggested that it
also includes FreeBSD 6.4:

https://www.gnu.org/software/gnulib/manual/html_node/realpath.html

[Greg Troxel]

Rob Browning <r...@defaultvalue.org> writes:

>> #if netbsd and version <= 5
>> #define OLD_REALPATH
>> #endif
>
> Maybe things have changed, but it looked like this suggested that it
> also includes FreeBSD 6.4:
>
> https://www.gnu.org/software/gnulib/manual/html_node/realpath.html

Yes, I saw that after I typed too quickly. I wrote the #define
OLD_REALPATH that way so that adding other conditionals would be easy
and non-confusing; it was clear there is some messy set. Using the
gnulib list as a starting point seems good.

[Rob Browning]

Greg Troxel <g...@lexort.com> writes:

> Yes, I saw that after I typed too quickly. I wrote the #define
> OLD_REALPATH that way so that adding other conditionals would be easy
> and non-confusing; it was clear there is some messy set. Using the
> gnulib list as a starting point seems good.

OK, right.

Be nice if there were some way we could reliably test dynamically
(e.g. during configure), but not sure there is. I can think of some
pretty ugly ways, but nothing I think it'd be wise to pursue :)

[Rob Browning]

Greg Troxel <g...@lexort.com> writes:

> Yes, I saw that after I typed too quickly. I wrote the #define
> OLD_REALPATH that way so that adding other conditionals would be easy
> and non-confusing; it was clear there is some messy set. Using the
> gnulib list as a starting point seems good.

Oh, and were you planning to working on a final patch, or did you want
me to finish it up (no rush, just wanted to make sure we weren't waiting
on me).

[Greg Troxel]

Rob Browning <r...@defaultvalue.org> writes:

> Greg Troxel <g...@lexort.com> writes:
>
>> Yes, I saw that after I typed too quickly. I wrote the #define
>> OLD_REALPATH that way so that adding other conditionals would be easy
>> and non-confusing; it was clear there is some messy set. Using the
>> gnulib list as a starting point seems good.
>
> OK, right.
>
> Be nice if there were some way we could reliably test dynamically
> (e.g. during configure), but not sure there is. I can think of some
> pretty ugly ways, but nothing I think it'd be wise to pursue :)

I think we could build a test program that uses NULL and call it, but it
may be better to just hardcore the OS list. If someone has problems
they can add their system, and this is basically older versions of BSD,
as far as I can tell.