Pádraig Brady wrote:
> > In this case, when confstr (_CS_PATH) is longer than expected,
> > I would prefer
> > - either an abort(),
Recall: abort() is the preferred way for a programmer to say "if this
situation occurs, I made a mistake, and I want to be notified".
> Best not do that in a lib I think.
Oh, that's the rule that I heard 25 years ago, when working on X11. libX11
had no single call to abort(). And was libX11 reliable, or was it buggy?
It was buggy, because when an application never crashes via abort(), only
via "bad event" or so, users have no way to produce useful bug reports.
GNU libiconv calls abort() in 173 places, and GNU libunistring in 220 places.
This provides enough incentive for a user to report a malfunction, and
then I fix the bug. It doesn't happen often, though: only once in a couple
of years.
So, I'll use abort() here too.
> > - or a 'struct string_buffer', so that we don't use heap allocation
> > in the normal case.
> We could use heap allocation only in the edge case where 256 is not enough,
Yes, but this would be extra code for a hypothetical case that never occurs
in practice. Because in all cases I've seen (except maybe macOS and NetBSD),
confstr(_CS_PATH) returns a static literal string from libc:
glibc:
static const char cs_path[] = CS_PATH;
string = cs_path;
string_len = sizeof (cs_path);
musl:
s = "/bin:/usr/bin";
macOS:
#define _PATH_STDPATH "/usr/bin:/bin:/usr/sbin:/sbin"
mib[0] = CTL_USER;
mib[1] = USER_CS_PATH;
if (sysctl(mib, 2, NULL, &tlen, NULL, 0) == -1)
return (CONFSTR_ERR_RET);
if (len != 0 && buf != NULL) {
if ((p = malloc(tlen)) == NULL)
return (CONFSTR_ERR_RET);
if (sysctl(mib, 2, p, &tlen, NULL, 0) == -1) {
FreeBSD:
#define _PATH_STDPATH "/usr/bin:/bin:/usr/sbin:/sbin"
p = _PATH_STDPATH;
NetBSD:
#define _PATH_STDPATH \
"/usr/bin:/bin:/usr/sbin:/sbin:/usr/pkg/bin:/usr/pkg/sbin:/usr/local/bin:/usr/local/sbin"
mib[0] = CTL_USER;
mib[1] = USER_CS_PATH;
if (sysctl(mib, 2, NULL, &tlen, NULL, 0) == -1)
return 0;
if (len != 0 && buf != NULL) {
if ((p = malloc(tlen)) == NULL)
return 0;
if (sysctl(mib, 2, p, &tlen, NULL, 0) == -1) {
OpenBSD:
#define _PATH_STDPATH
"/usr/bin:/bin:/usr/sbin:/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin"
return (strlcpy(buf, _PATH_STDPATH, len) + 1);
Solaris OpenIndiana:
if (__xpg6 & _C99SUSv3_XPG6_sysconf_version)
path = "/usr/xpg6/bin:/usr/xpg4/bin:/usr/ccs/bin:"
"/usr/bin:/opt/SUNWspro/bin";
else
path = "/usr/xpg4/bin:/usr/ccs/bin:/usr/bin:"
"/opt/SUNWspro/bin";
Cygwin:
{ls ("/bin:/usr/bin")}, /* _CS_PATH */
> or even xmalloca(), though I'd keep it as simple as possible for this
> extreme edge case.
xmalloca() would be essentially the same approach as the string-buffer
approach.
> If confstr returns 0, then we could use the glibc default of "/bin:/usr/bin"
> or maybe just return NULL, as it's also an extreme edge case.
This too is a hypothetical case. All known confstr() implementations
support _CS_PATH.
So, here's the patch that I will apply.
diff --git a/lib/findprog-in.c b/lib/findprog-in.c
index d937f7c834..1cc833316a 100644
--- a/lib/findprog-in.c
+++ b/lib/findprog-in.c
@@ -223,9 +223,15 @@ find_in_given_path (const char *progname, const char *path,
dependent. On POSIX systems, it is confstr (_CS_PATH). */
#if HAVE_CONFSTR
int ret = confstr (_CS_PATH, default_path, sizeof (default_path));
- if (ret == 0 || ret > sizeof (default_path))
+ if (ret == 0)
+ /* The platform has confstr(), but it does not support _CS_PATH. */
+ abort ();
+ if (ret > sizeof (default_path))
+ /* MAX_DEFAULT_PATH_SIZE is too small. Increase it. */
+ abort ();
+ #else /* native Windows, Android, Minix */
+ default_path[0] = '\0';
#endif
- default_path[0] = '\0';
path = default_path;
}
diff --git a/lib/findprog.c b/lib/findprog.c
index 2d321dc0d9..63264bd159 100644
--- a/lib/findprog.c
+++ b/lib/findprog.c
@@ -77,9 +77,15 @@ find_in_path (const char *progname)
dependent. On POSIX systems, it is confstr (_CS_PATH). */
#if HAVE_CONFSTR
int ret = confstr (_CS_PATH, default_path, sizeof (default_path));
- if (ret == 0 || ret > sizeof (default_path))
+ if (ret == 0)
+ /* The platform has confstr(), but it does not support _CS_PATH. */
+ abort ();
+ if (ret > sizeof (default_path))
+ /* MAX_DEFAULT_PATH_SIZE is too small. Increase it. */
+ abort ();
+ #else /* native Windows, Android, Minix */
+ default_path[0] = '\0';
#endif
- default_path[0] = '\0';
path = default_path;
}
if (*path == '\0')
