Date: Thu, 13 Aug 2015 13:09:41 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jilles Tjoelker <jilles@stack.nl> Cc: Baptiste Daroussin <bapt@freebsd.org>, src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: Re: svn commit: r286521 - projects/collation/lib/libc/locale Message-ID: <20150813121921.M1092@besplex.bde.org> In-Reply-To: <20150812201438.GA26529@stack.nl> References: <201508091150.t79Boo3v096088@repo.freebsd.org> <20150809223647.O2415@besplex.bde.org> <20150812201438.GA26529@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 12 Aug 2015, Jilles Tjoelker wrote:
> On Sun, Aug 09, 2015 at 10:54:15PM +1000, Bruce Evans wrote:
>> On Sun, 9 Aug 2015, Baptiste Daroussin wrote:
>
>>> Log:
>>> Use asprintf/free instead of snprintf
>
>> Why? It takes 3 times as much code, and immediately gave you a memory
>> leak when you wrote only twice as much. You fixed the memory leak in the
>> next commit, but it might not always be so easy to see.
>
>>> Modified: projects/collation/lib/libc/locale/collate.c
>>> ==============================================================================
>>> --- projects/collation/lib/libc/locale/collate.c Sun Aug 9 11:47:01 2015 (r286520)
>>> +++ projects/collation/lib/libc/locale/collate.c Sun Aug 9 11:50:50 2015 (r286521)
>>> @@ -107,7 +107,7 @@ int
>>> __collate_load_tables_l(const char *encoding, struct xlocale_collate *table)
>>> {
>>> int i, chains, z;
>>> - char buf[PATH_MAX];
>>> + char *buf;
>
>> POSIX_ME_HARDER code would use {PATH_MAX} = sysconf(__PATH_MAX) and error
>> handling for sysconf() and then for malloc()ing {PATH_MAX} bytes. This
>> would take 10 times as much code, except it could use a VLA with no
>> error checking for the allocation starting with C99. The asprintf()
>> method would be better then.
>
> No, POSIX_ME_HARDER code would use dynamic allocation (but without
> asprintf() since that's not in POSIX). This is because {PATH_MAX} may be
> indeterminate (pathconf() returns -1 but does not set errno).
Oops. I also mixed up pathconf() with sysconf(). So POSIX_ME_HARDER
actually takes 50 to 100 times as much code in its full glory. But since
it can return -1 with no error (meaning that the path length can be
anything), then if we don't care about efficiency we should ignore the
existence of pathconf() and use the same dynamic allocation that must
use to handle the case where pathconf() returns -1 with no error.
pathconf() instead of sysconf() also gives the complication that you
have to pass it a pathname and may need dynamic allocation to construct
that name. Here the pathname might be hard-coded as _PATH_DEV. That
used to work, but is now slightly broken, since we support the pts
subdirectory (but no other subdirectories), so the technically correct
pathname is _PATH_DEV concatenated with "pts/" in some cases.
> Note that some usages of certain functions are invalid when {PATH_MAX}
> is indeterminate, for example realpath() with a non-NULL resolved_path
> parameter.
Hmm. Is this spelled out in POSIX now? The 2001 version just says that
realpath() fails if the resolved path would be longer than {PATH_MAX}.
All paths are longer than -1, so if {PATH_MAX} is literally -1 in the
indeterminate case then your conclusion follows. POSIX says {PATH_MAX}
a lot in contexts where the indeterminate limit shouldn't be a problem
but -1 is a problem. Even comparing -1 with a path length is a problem.
Path lengths have type size_t and you get an unsign extension bug if
you compare -1 with a size_t.
> It seems uncommon to have a fixed {PATH_MAX} but not define it as a
> compile-time constant. An indeterminate {PATH_MAX} is used in GNU Hurd
> and its developers occasionally patch software to make it cope with
> that.
{PATH_MAX} is actually a compile-time constant in FreeBSD, unlike many
other misdefined constants ({NAME_MAX}, {OPEN_MAX}, ...)
> I think it is fine to use dynamic allocation here.
But why bother when there are thousands of other hard-coded MAXPATHLEN's
and PATH_MAX's in /usr/src? Before switching to dynamic allocation, add
error checking to thousands of snprintf()s. For a compile-time constant
like MAXPATHLEN, treating truncation of snprintf() writing into a buffer
os size MAXPATHLEN as an error is exactly correct, since if you could
construct a longer string then the syscall that uses it would fail.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150813121921.M1092>