This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] malloc: Implement heap protector
- From: Paul Eggert <eggert at cs dot ucla dot edu>
- To: Florian Weimer <fweimer at redhat dot com>, GNU C Library <libc-alpha at sourceware dot org>
- Date: Fri, 28 Oct 2016 10:55:51 -0700
- Subject: Re: [PATCH] malloc: Implement heap protector
- Authentication-results: sourceware.org; auth=none
- References: <dd818b8d-7176-8490-0780-e6e25756323a@redhat.com>
Thanks for working on this. The idea looks good. I second Carlos's
comments on comment and NEWS. Some other (minor) issues:
> +/* The heap cookie. The lowest three bits (corresponding to
> + SIZE_BITS) in __malloc_guard_header must be clear. Initialized
It's __malloc_header_guard, not __malloc_guard_header.
> + during libc startup, and computed by elf/dl-keysetup.c. */
> +INTERNAL_SIZE_T __malloc_header_guard; /* For size. */
> +INTERNAL_SIZE_T __malloc_footer_guard; /* For prev_size. */
Why two cookies? Doesn't one suffice, and mightn't that be a tad more
efficient? The comment should explain.
> +/* Decrypt a heap header chunk. */
These macros both encrypt and decrypt, right? At least, they're used
that way. The comment should say so.
Come to think of it, suppose we want to microimprove the implementation
to use val+guard to encrypt and val-guard to decrypt; this might be bit
faster on some platforms, and would be safe in unsigned arithmetic.
Would we then need two sets of macros, one for encryption and one for
decryption? Then perhaps we should have two sets of macros now, even
though the implementations are the same now, to make invoking code
clearer and more-portable to a +/- approach.
> +#define HEAP_CRYPT_SIZE(val) (__malloc_header_guard ^
((INTERNAL_SIZE_T) val))
> +#define HEAP_CRYPT_PREVSIZE(val) \
> + (__malloc_footer_guard ^ ((INTERNAL_SIZE_T) val))
'val' needs to be parenthesized in these two macro definientia.
> +#define set_foot(p, s) \
> + (((mchunkptr) ((char *) (p) + (s)))->mchunk_prev_size \
> + = HEAP_CRYPT_PREVSIZE (s))
Last line is indented one space too much.
> + else if (usable > size + 4096)
Rephrase as 'usable - size > 4096' so that we don't have to worry about
SIZE being ridiculously large. The subtraction cannot possibly overflow
here.
> + are treated as fake mmapped chunks. We cannot use the regular
> + accessors because the chunks we read are not yet encrypted. */
Avoid the royal "we", and use imperative voice instead: "We cannot use"
-> "Don't use", "the chunks we read" -> "the chunks read".
> + /* We treat all chunks as allocated.
...
> + /* In the non-shared case, we initialize the heap guard
> + directly. */
...
> + /* For a shared library, elf/rtld.c performed key setup in
> + security_init, and we copy the keys. In static builds, the guard
...
> +/* After heap initialization, we can call malloc_usable_size to check
> + if it gives valid results. */
...
> + /* The heap has been initialized. We may now call
> + malloc_usable_size. */
Similarly, avoid "we".