From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12706 Path: news.gmane.org!.POSTED!not-for-mail From: Alexander Monakov Newsgroups: gmane.linux.lib.musl.general Subject: Re: [RFC PATCH] reduce severity of ldso reclaim_gaps hack Date: Thu, 12 Apr 2018 00:19:24 +0300 (MSK) Message-ID: References: <20170628165243.16502-1-amonakov@ispras.ru> <20180411201057.GO3094@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 1523481475 29191 195.159.176.226 (11 Apr 2018 21:17:55 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 11 Apr 2018 21:17:55 +0000 (UTC) User-Agent: Alpine 2.20.13 (LNX 116 2015-12-14) To: musl@lists.openwall.com Original-X-From: musl-return-12722-gllmg-musl=m.gmane.org@lists.openwall.com Wed Apr 11 23:17:51 2018 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 1f6N7n-0007Sk-Jz for gllmg-musl@m.gmane.org; Wed, 11 Apr 2018 23:17:51 +0200 Original-Received: (qmail 9663 invoked by uid 550); 11 Apr 2018 21:19:59 -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 9640 invoked from network); 11 Apr 2018 21:19:59 -0000 In-Reply-To: <20180411201057.GO3094@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:12706 Archived-At: > > @@ -410,10 +412,9 @@ void *realloc(void *p, size_t n) > > size_t newlen = n + extra; > > /* Crash on realloc of freed chunk */ > > if (extra & 1) a_crash(); > > - if (newlen < PAGE_SIZE && (new = malloc(n))) { > > - memcpy(new, p, n-OVERHEAD); > > - free(p); > > - return new; > > + if (newlen < PAGE_SIZE && (new = malloc(n-OVERHEAD))) { > > + n0 = n; > > + goto copy_free_ret; > > } > > newlen = (newlen + PAGE_SIZE-1) & -PAGE_SIZE; > > if (oldlen == newlen) return p; > > @@ -456,34 +457,20 @@ copy_realloc: > > /* As a last resort, allocate a new chunk and copy to it. */ > > new = malloc(n-OVERHEAD); > > if (!new) return 0; > > +copy_free_ret: > > memcpy(new, p, n0-OVERHEAD); > > free(CHUNK_TO_MEM(self)); > > return new; > > } > > This looks like an independent change that fixes a separate > slight-overallocation bug. Is it related? No, it would be nicer to commit it separately. > > +#if defined(__GNUC__) > > +__attribute__((cold)) > > +#endif > > This can't be used as-is. It would need a configure check (gcc version > dependent) and __cold__ if we want to, but unless there's a strong > reason to include it I'd just omit it. malloc.c is compiled with -O3, causing 'free' to be inlined in all local callers. This combats code growth and should also slightly improve code layout in the new 'free' (not that it would help much, of course). > > +static void unmap_chunk(struct chunk *self) > > +{ > > + size_t extra = self->psize; > > + char *base = (char *)self - extra; > > + size_t len = CHUNK_SIZE(self) + extra; > > + /* Crash on double free */ > > + if (extra & 1) a_crash(); > > + __munmap(base, len); > > +} > > + > > +void free(void *p) > > +{ > > + if (!p) return; > > + > > + struct chunk *self = MEM_TO_CHUNK(p); > > + > > + if (IS_MMAPPED(self)) > > + unmap_chunk(self); > > + else > > + bin_chunk(self); > > +} > > + > > +void __malloc_donate(char *start, char *end) > > +{ > > It's confusing to see that this is equivalent to what's being removed > from dynlink.c, but I think it may be correct. > > > + if (end - start < 2*OVERHEAD + SIZE_ALIGN) return; > > This does not seem like a sufficient bound to ensure the block is > usable, but the next check after alignment may cover it. Yes, this is to ensure that we don't create invalid pointers when aligning, > > + start += OVERHEAD + SIZE_ALIGN - 1; > > + start -= (uintptr_t)start & (SIZE_ALIGN - 1); > > This looks correct. > > > + end -= (uintptr_t)end & (SIZE_ALIGN - 1); > > This does not subtract the OVERHEAD, but I think it's just a > notational difference; my "end" pointed to the end of the chunk to be > freed, and your "end" points to the beginning of the next > (nonexistent) chunk. The MEM_TO_CHUNK below should compensate. > > > + if (end - start < OVERHEAD + SIZE_ALIGN) return; > > At this point, start and end both point just past a chunk header, > meaning they have to differ by a multiple of SIZE_ALIGN. I don't see > why OVERHEAD is needed here too. The check should be against > SIZE_ALIGN I think (although by alignment they're equivalent). I don't recall if I had a specific reason to spell it like that. > > + if (end - start >= MMAP_THRESHOLD) return; > > This does not seem necessary. Free chunks in the last bin can be > larger than MMAP_THRESHOLD; they're just broken up to satisfy > allocations. Of course it's unlikely to happen anyway. Do such oversized chunks appear in normal operation? This seems non-obvious, so a comment pointing that out would probably be helpful. Thanks. Alexander