Re: malloc: handle to be cleaned chunks the same as regular ones
On Fri, Feb 24, 2023 at 08:13:13AM +0100, Otto Moerbeek wrote:
> On Sat, Feb 18, 2023 at 04:12:08PM +0100, Otto Moerbeek wrote:
>
> > Hi,
> >
> > these recent sshd double free issue prompted me to look at malloc
> > again. I have something bigger brewing, but this diff makes sure the
> > to be cleaned chunks (via freezero() or malloc_conceal) particpate in
> > the delayed freeing just as others.
>
> I'd like to see tests/reviews of this.
I have tested this for a few days on my main machines (amd64 and arm64).
I also tested it on sparc64 and i386 (machines mostly used for building).
The diff reads fine
ok tb
>
> >
> > -Otto
> >
> > Index: stdlib/malloc.c
> > ===
> > RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
> > retrieving revision 1.276
> > diff -u -p -r1.276 malloc.c
> > --- stdlib/malloc.c 27 Dec 2022 17:31:09 - 1.276
> > +++ stdlib/malloc.c 16 Feb 2023 14:11:50 -
> > @@ -1515,42 +1515,38 @@ ofree(struct dir_info **argpool, void *p
> > unmap(pool, p, PAGEROUND(sz), clear ? argsz : 0);
> > delete(pool, r);
> > } else {
> > + void *tmp;
> > + u_int i;
> > +
> > /* Validate and optionally canary check */
> > struct chunk_info *info = (struct chunk_info *)r->size;
> > if (info->size != sz)
> > wrterror(pool, "internal struct corrupt");
> > find_chunknum(pool, info, p, mopts.chunk_canaries);
> > - if (!clear) {
> > - void *tmp;
> > - int i;
> >
> > - if (mopts.malloc_freecheck) {
> > - for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
> > - if (p == pool->delayed_chunks[i])
> > - wrterror(pool,
> > - "double free %p", p);
> > - }
> > - junk_free(pool->malloc_junk, p, sz);
> > - i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
> > - tmp = p;
> > - p = pool->delayed_chunks[i];
> > - if (tmp == p)
> > - wrterror(pool, "double free %p", tmp);
> > - pool->delayed_chunks[i] = tmp;
> > - if (p != NULL) {
> > - r = find(pool, p);
> > - REALSIZE(sz, r);
> > - if (r != NULL)
> > - validate_junk(pool, p, sz);
> > - }
> > - } else if (argsz > 0) {
> > - r = find(pool, p);
> > - explicit_bzero(p, argsz);
> > + if (mopts.malloc_freecheck) {
> > + for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
> > + if (p == pool->delayed_chunks[i])
> > + wrterror(pool,
> > + "double free %p", p);
> > }
> > + if (clear && argsz > 0)
> > + explicit_bzero(p, argsz);
> > + junk_free(pool->malloc_junk, p, sz);
> > +
> > + i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
> > + tmp = p;
> > + p = pool->delayed_chunks[i];
> > + if (tmp == p)
> > + wrterror(pool, "double free %p", p);
> > + pool->delayed_chunks[i] = tmp;
> > if (p != NULL) {
> > + r = find(pool, p);
> > if (r == NULL)
> > wrterror(pool,
> > "bogus pointer (double free?) %p", p);
> > + REALSIZE(sz, r);
> > + validate_junk(pool, p, sz);
> > free_bytes(pool, r, p);
> > }
> > }
> >
>
Re: malloc: handle to be cleaned chunks the same as regular ones
On Sat, Feb 18, 2023 at 04:12:08PM +0100, Otto Moerbeek wrote:
> Hi,
>
> these recent sshd double free issue prompted me to look at malloc
> again. I have something bigger brewing, but this diff makes sure the
> to be cleaned chunks (via freezero() or malloc_conceal) particpate in
> the delayed freeing just as others.
I'd like to see tests/reviews of this.
>
> -Otto
>
> Index: stdlib/malloc.c
> ===
> RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.276
> diff -u -p -r1.276 malloc.c
> --- stdlib/malloc.c 27 Dec 2022 17:31:09 - 1.276
> +++ stdlib/malloc.c 16 Feb 2023 14:11:50 -
> @@ -1515,42 +1515,38 @@ ofree(struct dir_info **argpool, void *p
> unmap(pool, p, PAGEROUND(sz), clear ? argsz : 0);
> delete(pool, r);
> } else {
> + void *tmp;
> + u_int i;
> +
> /* Validate and optionally canary check */
> struct chunk_info *info = (struct chunk_info *)r->size;
> if (info->size != sz)
> wrterror(pool, "internal struct corrupt");
> find_chunknum(pool, info, p, mopts.chunk_canaries);
> - if (!clear) {
> - void *tmp;
> - int i;
>
> - if (mopts.malloc_freecheck) {
> - for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
> - if (p == pool->delayed_chunks[i])
> - wrterror(pool,
> - "double free %p", p);
> - }
> - junk_free(pool->malloc_junk, p, sz);
> - i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
> - tmp = p;
> - p = pool->delayed_chunks[i];
> - if (tmp == p)
> - wrterror(pool, "double free %p", tmp);
> - pool->delayed_chunks[i] = tmp;
> - if (p != NULL) {
> - r = find(pool, p);
> - REALSIZE(sz, r);
> - if (r != NULL)
> - validate_junk(pool, p, sz);
> - }
> - } else if (argsz > 0) {
> - r = find(pool, p);
> - explicit_bzero(p, argsz);
> + if (mopts.malloc_freecheck) {
> + for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
> + if (p == pool->delayed_chunks[i])
> + wrterror(pool,
> + "double free %p", p);
> }
> + if (clear && argsz > 0)
> + explicit_bzero(p, argsz);
> + junk_free(pool->malloc_junk, p, sz);
> +
> + i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
> + tmp = p;
> + p = pool->delayed_chunks[i];
> + if (tmp == p)
> + wrterror(pool, "double free %p", p);
> + pool->delayed_chunks[i] = tmp;
> if (p != NULL) {
> + r = find(pool, p);
> if (r == NULL)
> wrterror(pool,
> "bogus pointer (double free?) %p", p);
> + REALSIZE(sz, r);
> + validate_junk(pool, p, sz);
> free_bytes(pool, r, p);
> }
> }
>
malloc: handle to be cleaned chunks the same as regular ones
Hi,
these recent sshd double free issue prompted me to look at malloc
again. I have something bigger brewing, but this diff makes sure the
to be cleaned chunks (via freezero() or malloc_conceal) particpate in
the delayed freeing just as others.
-Otto
Index: stdlib/malloc.c
===
RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.276
diff -u -p -r1.276 malloc.c
--- stdlib/malloc.c 27 Dec 2022 17:31:09 - 1.276
+++ stdlib/malloc.c 16 Feb 2023 14:11:50 -
@@ -1515,42 +1515,38 @@ ofree(struct dir_info **argpool, void *p
unmap(pool, p, PAGEROUND(sz), clear ? argsz : 0);
delete(pool, r);
} else {
+ void *tmp;
+ u_int i;
+
/* Validate and optionally canary check */
struct chunk_info *info = (struct chunk_info *)r->size;
if (info->size != sz)
wrterror(pool, "internal struct corrupt");
find_chunknum(pool, info, p, mopts.chunk_canaries);
- if (!clear) {
- void *tmp;
- int i;
- if (mopts.malloc_freecheck) {
- for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
- if (p == pool->delayed_chunks[i])
- wrterror(pool,
- "double free %p", p);
- }
- junk_free(pool->malloc_junk, p, sz);
- i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
- tmp = p;
- p = pool->delayed_chunks[i];
- if (tmp == p)
- wrterror(pool, "double free %p", tmp);
- pool->delayed_chunks[i] = tmp;
- if (p != NULL) {
- r = find(pool, p);
- REALSIZE(sz, r);
- if (r != NULL)
- validate_junk(pool, p, sz);
- }
- } else if (argsz > 0) {
- r = find(pool, p);
- explicit_bzero(p, argsz);
+ if (mopts.malloc_freecheck) {
+ for (i = 0; i <= MALLOC_DELAYED_CHUNK_MASK; i++)
+ if (p == pool->delayed_chunks[i])
+ wrterror(pool,
+ "double free %p", p);
}
+ if (clear && argsz > 0)
+ explicit_bzero(p, argsz);
+ junk_free(pool->malloc_junk, p, sz);
+
+ i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
+ tmp = p;
+ p = pool->delayed_chunks[i];
+ if (tmp == p)
+ wrterror(pool, "double free %p", p);
+ pool->delayed_chunks[i] = tmp;
if (p != NULL) {
+ r = find(pool, p);
if (r == NULL)
wrterror(pool,
"bogus pointer (double free?) %p", p);
+ REALSIZE(sz, r);
+ validate_junk(pool, p, sz);
free_bytes(pool, r, p);
}
}
