This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] malloc: Implement heap protector


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".


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]