Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 17 Apr 2018 08:30:40 +0300 (MSK)
From: Alexander Monakov <amonakov@...ras.ru>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 2/2] ldso, malloc: implement reclaim_gaps via
 __malloc_donate

> > +void __malloc_donate(char *start, char *end)
> > +{
> > +	ssize_t align_start_up = (SIZE_ALIGN - 1) & -(uintptr_t)start;
> > +	ssize_t align_end_down = (SIZE_ALIGN - 1) &  (uintptr_t)end;
> > +	ssize_t chunk_size = end - start - (OVERHEAD + align_start_up + align_end_down);
> > +	if (chunk_size < OVERHEAD + SIZE_ALIGN) return;
> > +	start += align_start_up;
> > +	end   -= align_end_down;
> > +
> > +	struct chunk *c = MEM_TO_CHUNK(start + OVERHEAD), *n = MEM_TO_CHUNK(end);
> > +	c->psize = n->csize = C_INUSE;
> > +	c->csize = n->psize = C_INUSE | chunk_size;
> > +	bin_chunk(c);
> > +}
> 
> I think this version of the size logic is harder to read than the old
> one, and inconsistent with how malloc does accounting internally. In
> the notation used everywhere else, "chunk size" always includes
> OVERHEAD plus the usable space; it's the distance between the chunk
> header and the next chunk header.

Same here. In 'ssize_t chunk_size = ...', OVERHEAD is subtracted once to
account for the sentinel/footer; to compute usable size, OVERHEAD would
need to be subtracted twice.

> I also don't like use of signed arithmetic with sizes. I thought all
> the values were nonnegative anyway, so I wasn't sure why you
> introduced signed types, but apparently chunk_size can be negative and
> the comparison against OVERHEAD+SIZE_ALIGN relies on the RHS being
> signed (and on the unsigned result of the expression initializing
> chunk_size getting coerced to signed) to work.

Well, 'end - start' is a signed expression to begin with, so I doubt a
coercion is taking place there.

Is there a problem with assuming OVERHEAD+SIZE_ALIGN is signed?

> I think the above code may also be wrong. start is aligned mod
> SIZE_ALIGN, so start+OVERHEAD is misaligned, and therefore not a valid
> argument to MEM_TO_CHUNK. Continuing further along this line of
> reasoning, aligning start up mod SIZE_ALIGN like you're doing is not
> sufficient. There must also be space for a header below the aligned
> point.

Yes, that's an oversight, but easily corrected: we need to adjust 'start'
so it's congruent to -OVERHEAD (rather than 0) modulo SIZE_ALIGN:

void __malloc_donate(char *start, char *end)
{
	ssize_t align_start_up = (SIZE_ALIGN - 1) & (-(uintptr_t)start - OVERHEAD);
	ssize_t align_end_down = (SIZE_ALIGN - 1) & (uintptr_t)end;
	ssize_t chunk_size = end - start - (OVERHEAD + align_start_up + align_end_down);
	if (chunk_size < OVERHEAD + SIZE_ALIGN) return;
	start += align_start_up;
	end   -= align_end_down;

	struct chunk *c = MEM_TO_CHUNK(start + OVERHEAD), *n = MEM_TO_CHUNK(end);
	c->psize = n->csize = C_INUSE;
	c->csize = n->psize = C_INUSE | chunk_size;
	bin_chunk(c);
}

The above addresses the alignment issue, and I've responded to other
concerns. Do you need a new patch with this?

Alexander

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.