Date: Wed, 14 Dec 2005 21:48:59 +0100 From: Daniel Hartmeier <daniel@benzedrine.cx> To: freebsd-threads@freebsd.org Subject: Re: threads/90392: libc stdio memory leak with pthread Message-ID: <20051214204859.GJ17140@insomnia.benzedrine.cx> In-Reply-To: <20051214162744.GI17140@insomnia.benzedrine.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
It's actually a waste to lock the mutex and destroy it again, since it's really private to sscanf() and only provided as part of the API and not because it serves a real locking purpose in this case. The patch below splits fread() into a MT-safe and Non-MT-safe version, so __svfscanf() can call the latter. This is in the spirit of how many other stdio functions were split, for instance see the diff and commit message of rev. 1.8 of ungetc.c http://www.freebsd.org/cgi/cvsweb.cgi/src/lib/libc/stdio/ungetc.c With this, sscanf() never triggers a mutex allocation or lock, and there's no memory leak due to lacking pthread_mutex_destroy(). I checked all the other uses of INITEXTRA(), and none of them share this problem, since in those cases only the non-locking functions (with prefix __) are called. There might some risk of someone later introducing calls to locking functions, but that fread() call in __svfscanf() was really the only offender I can spot. You could add the pthread_mutex_destroy() as suggested in the first patch additionally, to cover future mistakes. Daniel --- local.h.orig Wed Dec 14 20:29:56 2005 +++ local.h Wed Dec 14 20:31:44 2005 @@ -79,6 +79,8 @@ extern int __vfwprintf(FILE *, const wchar_t *, __va_list); extern int __vfwscanf(FILE * __restrict, const wchar_t * __restrict, __va_list); +extern size_t __fread(void * __restrict buf, size_t size, size_t count, + FILE * __restrict fp); extern int __sdidinit; --- fread.c.orig Wed Dec 14 20:22:31 2005 +++ fread.c Wed Dec 14 20:29:11 2005 @@ -47,11 +47,25 @@ #include "local.h" #include "libc_private.h" +/* + * MT-safe version + */ size_t -fread(buf, size, count, fp) - void * __restrict buf; - size_t size, count; - FILE * __restrict fp; +fread(void * __restrict buf, size_t size, size_t count, FILE * __restrict fp) +{ + int ret; + + FLOCKFILE(fp); + ret = __fread(buf, size, count, fp); + FUNLOCKFILE(fp); + return (ret); +} + +/* + * Non-MT-safe version + */ +size_t +__fread(void * __restrict buf, size_t size, size_t count, FILE * __restrict fp) { size_t resid; char *p; @@ -65,7 +79,6 @@ */ if ((resid = count * size) == 0) return (0); - FLOCKFILE(fp); ORIENT(fp, -1); if (fp->_r < 0) fp->_r = 0; @@ -79,13 +92,11 @@ resid -= r; if (__srefill(fp)) { /* no more input: return partial result */ - FUNLOCKFILE(fp); return ((total - resid) / size); } } (void)memcpy((void *)p, (void *)fp->_p, resid); fp->_r -= resid; fp->_p += resid; - FUNLOCKFILE(fp); return (count); } --- vfscanf.c.orig Wed Dec 14 20:22:53 2005 +++ vfscanf.c Wed Dec 14 20:32:08 2005 @@ -412,7 +412,7 @@ } nread += sum; } else { - size_t r = fread((void *)va_arg(ap, char *), 1, + size_t r = __fread((void *)va_arg(ap, char *), 1, width, fp); if (r == 0)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20051214204859.GJ17140>