From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/11384 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: Use-after-free in __unlock Date: Thu, 1 Jun 2017 12:16:38 -0400 Message-ID: <20170601161638.GL1627@brightrain.aerifal.cx> References: <20170601155753.GK1627@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1496333811 24745 195.159.176.226 (1 Jun 2017 16:16:51 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 1 Jun 2017 16:16:51 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-11397-gllmg-musl=m.gmane.org@lists.openwall.com Thu Jun 01 18:16:48 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1dGSmF-0006AC-5u for gllmg-musl@m.gmane.org; Thu, 01 Jun 2017 18:16:47 +0200 Original-Received: (qmail 18421 invoked by uid 550); 1 Jun 2017 16:16:50 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 18403 invoked from network); 1 Jun 2017 16:16:50 -0000 Content-Disposition: inline In-Reply-To: <20170601155753.GK1627@brightrain.aerifal.cx> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:11384 Archived-At: On Thu, Jun 01, 2017 at 11:57:53AM -0400, Rich Felker wrote: > Thanks for finding and reporting this. Indeed, __lock and __unlock are > not made safe for use on dynamic-lifetime objects, and I was wrongly > thinking they were only used for static-lifetime ones (various > libc-internal locks). > > For infrequently-used locks, which these seem to be, I see no reason > to optimize for contention by having a separate waiter count; simply > having a single atomic int whose states are "unlocked", "locked with > no waiters", and "locked maybe with waiters" is a simple solution and > slightly shrinks the relevant structures anyway. It's possible that > this is the right solution for all places where __lock and __unlock > are used, but we should probably do a review and see which ones might > reasonably be subject to high contention (where spurious FUTEX_WAKE is > very costly). Quick analysis (based on search for ./-> inside __unlock/UNLOCK args): Places where __lock/LOCK is used incorrectly: - pthread struct - DIR objects (maybe) And places where it's used correctly: - tz - random - syslog - static-linked no-free version of malloc - gettext - locale - pthread_atfork - sem_open - synccall - atexit & variants - open file list Of the latter, these seem like things you could reasonably hammer where false contention would be costly: - tz - random - gettext There are others where you could hammer, but they already involve much heavier syscalls than futex so it's unlikely to matter what lock contention tracking method is used. I actually don't think the current "incorrect" use in DIR objects matters, since there's no way to observe that thread B can legally close a dir until _after_ thread A's call that took the lock returns and thread A synchronizes with thread B. That is, mere use of __lock/__unlock in dynamic-storage objects is not actually wrong; the problem is specific to pthread exit lock and the related logic concerning object lifetime. So a local fix may actually be more appropriate. Rich