From owner-freebsd-threads@FreeBSD.ORG Wed Dec 14 20:49:28 2005 Return-Path: X-Original-To: freebsd-threads@freebsd.org Delivered-To: freebsd-threads@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 64E7E16A420 for ; Wed, 14 Dec 2005 20:49:28 +0000 (GMT) (envelope-from dhartmei@insomnia.benzedrine.cx) Received: from insomnia.benzedrine.cx (insomnia.benzedrine.cx [62.65.145.30]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1503F43D9D for ; Wed, 14 Dec 2005 20:49:07 +0000 (GMT) (envelope-from dhartmei@insomnia.benzedrine.cx) Received: from insomnia.benzedrine.cx (dhartmei@localhost [127.0.0.1]) by insomnia.benzedrine.cx (8.13.4/8.12.11) with ESMTP id jBEKn10X022972 (version=TLSv1/SSLv3 cipher=DHE-DSS-AES256-SHA bits=256 verify=NO) for ; Wed, 14 Dec 2005 21:49:01 +0100 (MET) Received: (from dhartmei@localhost) by insomnia.benzedrine.cx (8.13.4/8.12.10/Submit) id jBEKn0Ex005096 for freebsd-threads@freebsd.org; Wed, 14 Dec 2005 21:49:00 +0100 (MET) Date: Wed, 14 Dec 2005 21:48:59 +0100 From: Daniel Hartmeier To: freebsd-threads@freebsd.org Message-ID: <20051214204859.GJ17140@insomnia.benzedrine.cx> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20051214162744.GI17140@insomnia.benzedrine.cx> User-Agent: Mutt/1.5.10i Subject: Re: threads/90392: libc stdio memory leak with pthread X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Dec 2005 20:49:28 -0000 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)