Date: Sun, 9 Aug 2015 22:54:15 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Baptiste Daroussin <bapt@freebsd.org> Cc: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: Re: svn commit: r286521 - projects/collation/lib/libc/locale Message-ID: <20150809223647.O2415@besplex.bde.org> In-Reply-To: <201508091150.t79Boo3v096088@repo.freebsd.org> References: <201508091150.t79Boo3v096088@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
> char *TMP;
> char *map;
> collate_info_t *info;
> @@ -120,11 +120,13 @@ __collate_load_tables_l(const char *enco
> return (_LDP_CACHE);
> }
>
> - (void) snprintf(buf, sizeof (buf), "%s/%s/LC_COLLATE",
> - _PathLocale, encoding);
> + asnprintf(&buf, "%s/%s/LC_COLLATE", _PathLocale, encoding);
> + if (buf == NULL)
> + return (_LDP_ERROR);
There was no error checking for snprintf(). In fact, its value was
voided. But error checking for snprintf() can be added just as easily
as error checking for asprintf().
I think this code doesn't compile, because asnprintf() makes no sense
and doesn't exist.
>
> if ((fd = _open(buf, O_RDONLY)) < 0)
Memory leak here.
> return (_LDP_ERROR);
> + free(buf);
The memory leak can be fixed better than in the committed version using:
fd = _open(...);
free(buf);
if (fd < 0)
return (_LDP_ERROR);
> if (_fstat(fd, &sbuf) < 0) {
> (void) _close(fd);
> return (_LDP_ERROR);
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150809223647.O2415>