From owner-freebsd-hackers@FreeBSD.ORG  Thu Dec 20 12:42:58 2012
Return-Path: <owner-freebsd-hackers@FreeBSD.ORG>
Delivered-To: freebsd-hackers@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52])
 by hub.freebsd.org (Postfix) with ESMTP id A1928B2C;
 Thu, 20 Dec 2012 12:42:58 +0000 (UTC) (envelope-from jilles@stack.nl)
Received: from mx1.stack.nl (unknown [IPv6:2001:610:1108:5012::107])
 by mx1.freebsd.org (Postfix) with ESMTP id 3A92C8FC0C;
 Thu, 20 Dec 2012 12:42:58 +0000 (UTC)
Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131])
 by mx1.stack.nl (Postfix) with ESMTP id 9958E1203C7;
 Thu, 20 Dec 2012 13:42:54 +0100 (CET)
Received: by snail.stack.nl (Postfix, from userid 1677)
 id 854B62848C; Thu, 20 Dec 2012 13:42:54 +0100 (CET)
Date: Thu, 20 Dec 2012 13:42:54 +0100
From: Jilles Tjoelker <jilles@stack.nl>
To: Dimitry Andric <dim@FreeBSD.org>
Subject: Re: use after free in grep?
Message-ID: <20121220124254.GA99616@stack.nl>
References: <CAF6rxg=Ni2Kcgdw2XrSVtU1f9eHaFt1-oBTNv8pm8An52x13nQ@mail.gmail.com>
 <50D3023B.8090407@FreeBSD.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <50D3023B.8090407@FreeBSD.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: Eitan Adler <lists@eitanadler.com>, Gabor Kovesdan <gabor@freebsd.org>,
 FreeBSD Hackers <freebsd-hackers@freebsd.org>
X-BeenThere: freebsd-hackers@freebsd.org
X-Mailman-Version: 2.1.14
Precedence: list
List-Id: Technical Discussions relating to FreeBSD
 <freebsd-hackers.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/freebsd-hackers>, 
 <mailto:freebsd-hackers-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-hackers>
List-Post: <mailto:freebsd-hackers@freebsd.org>
List-Help: <mailto:freebsd-hackers-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-hackers>,
 <mailto:freebsd-hackers-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 20 Dec 2012 12:42:58 -0000

On Thu, Dec 20, 2012 at 01:19:07PM +0100, Dimitry Andric wrote:
> On 2012-12-20 08:13, Eitan Adler wrote:
> > in xrealloc_impl

> > 338   new_ptr = realloc(ptr, new_size);
> > 339   if (new_ptr != NULL)
> > 340     {
> > 341       hash_table_del(xmalloc_table, ptr);

> > ^^^ isn't this a use-after-free of ptr?

> Yes, realloc does not guarantee the realloc'd space will be at the same
> address, so it may free ptr at its discretion.

Even if you somehow know realloc() is not going to move the block, it is
still wrong to use any pointer not derived from its return value to
access the block. Comparing the old and the new pointers (normally or
with memcmp()) does not help; it has an indeterminate result.

See http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_260.htm

> Also, there is a memory leak if realloc() returns NULL.  This is a
> very usual mistake when using realloc(). :-)

No, this would be correct if a successful realloc() call did not make
the old pointer indeterminate. The hash table remains unchanged if
realloc() fails.

> Probably, the code should do the hash_table_del() before the realloc(),
> but I am not sure if hash_table_del() will already free ptr.

Yes, and add it back if realloc() fails.

A smarter internal interface to the hash table would avoid freeing and
reallocating hash table entries here (which might fail).

-- 
Jilles Tjoelker