Re: [PATCH] malloc: Implement heap protector

[Paul Eggert <eggert@cs.ucla.edu>]

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