This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PING][PATCH][BZ #15073] Fix race in free.
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: Maxim Kuvyrkov <maxim at kugelworks dot com>
- Cc: GLIBC Devel <libc-alpha at sourceware dot org>
- Date: Sat, 21 Dec 2013 22:07:50 +0100
- Subject: Re: [PING][PATCH][BZ #15073] Fix race in free.
- Authentication-results: sourceware.org; auth=none
- References: <20131210194349 dot GA19644 at domone dot podge> <20131218102757 dot GC6559 at domone dot podge> <C593C68D-FF0C-4C46-93E2-4A3ABA33E84B at kugelworks dot com>
On Sun, Dec 22, 2013 at 09:08:42AM +1300, Maxim Kuvyrkov wrote:
> On 18/12/2013, at 11:27 pm, OndÅej BÃlka <neleai@seznam.cz> wrote:
>
> > ping
> > On Tue, Dec 10, 2013 at 08:43:49PM +0100, OndÅej BÃlka wrote:
> >> Hi,
> >>
> >> While actual implementation of atomic returning to fastbin is correct
> >> this could not be sayed about 'sanity' check surrounding it.
> >>
> >> When we read fastbin entry there is no quarantee that memory will be
> >> deallocated in meantime.
> >>
> >> Yet we try to dereference that pointer for a check that does not make
> >> any sense. We check if old chunk belongs to fastbin we are adding
> >> element. If old gets consolidated to bigger chunk we get a spurious
> >> memory corruption error, while if we continue a subsequent compare and
> >> swap will fail.
>
> Ondrej, good job on analyzing the bug. All above is exactly right.
>
> >>
> >> Solution is simply drop that check.
>
> Or fix the check. If we have_lock for the memory state there is no way a competing thread can snatch and deallocate the fastbin chunk that we are adding to.
>
As most malloc checks decrease readibility I favor deletion. Adding a
check for lock solves problem but as _int_free always called from
__libc_free with have_lock = 0 it would render check mostly useless.
> >>
> >> [BZ #15073]
> >> * malloc/malloc.c (_int_free): Remove sanity check that causes
> >> race condition.
> >>
> >> diff --git a/malloc/malloc.c b/malloc/malloc.c
> >> index b1668b5..a05d4bb 100644
> >> --- a/malloc/malloc.c
> >> +++ b/malloc/malloc.c
> >> @@ -3785,7 +3785,6 @@ _int_free(mstate av, mchunkptr p, int have_lock)
> >>
> >> mchunkptr fd;
> >> mchunkptr old = *fb;
> >> - unsigned int old_idx = ~0u;
> >> do
> >> {
> >> /* Another simple check: make sure the top of the bin is not the
> >> @@ -3795,13 +3794,10 @@ _int_free(mstate av, mchunkptr p, int have_lock)
> >> errstr = "double free or corruption (fasttop)";
> >> goto errout;
> >> }
> >> - if (old != NULL)
> >> - old_idx = fastbin_index(chunksize(old));
> >> p->fd = fd = old;
> >> }
> >> while ((old = catomic_compare_and_exchange_val_rel (fb, p, fd)) != fd);
> >>
> >> - if (fd != NULL && __builtin_expect (old_idx != idx, 0))
> >> + if (fd != NULL)
> >> {
> >> errstr = "invalid fastbin entry (free)";
> >> goto errout;
>
> Surely you intended to remove the whole if-clause. As it stands above, the code will produce spurious "invalid fastbin entry (free)" errors for any fastbin of size 2 or more.
>
Yes, I meant that.
> Also, as a general plea to community, please write comments and annotate any code that you fix. With glibc's smart and fast algorithms and implementations comments are invaluable.
>
> Ondrej, how about the following patch instead? Does it look correct to you?
>
Correct yes, question is how that check is useful.