[Swan-dev] pfree checks

[Antony Antony]

I link with Electric Fence. It will detect double free, and cause 
Segmentation fault. It kicks in before libreswan magic is executed. 
 
Here is an example. The passert in pfree(), before your patch is applied, do 
not provide any extra info in when linked with efnece. 

I just tried a double free, forced.

(gdb) bt
#0  0x0000559283461279 in pfree (ptr=0x7f36f14a4fe8) at 
/home/build/libreswan/lib/libswan/alloc.c:132
#1  0x00005592833e5db7 in free_pluto_main () at 
/home/build/libreswan/programs/pluto/plutomain.c:161
#2  0x00005592833e8dc3 in exit_pluto (status=0) at 
/home/build/libreswan/programs/pluto/plutomain.c:1858
#3  0x0000559283444b90 in whack_handle (whackctlfd=4)
    at /home/build/libreswan/programs/pluto/rcv_whack.c:750
#4  0x0000559283444908 in whack_handle_cb (fd=4, event=2, arg=0x0)

Note the passert, libreswan magic stuff we added did not execute.

If we go down path to add layers magical things to protect double free they 
seems to compete and resulting logs will depend how many layres active.  
This confuse me, just sharing my thought.  

-antony

On Sat, Oct 07, 2017 at 01:29:26PM -0400, Andrew Cagney wrote:
> I'm considering a commit containing some of the below additional checks:
> 
> - try to spot a double free (I think this check should go in)
> 
> - try to spot a use-after-free, not so sure about this
> 
> - and there is also the possibility of a check for heap overflow
> 
> but these start to stray into checks that free() is doing anyway :-/
> 
> thoughts,
> Andrew
> 
> diff --git a/lib/libswan/alloc.c b/lib/libswan/alloc.c
> index ab6f3194b..c1f9824e7 100644
> --- a/lib/libswan/alloc.c
> +++ b/lib/libswan/alloc.c
> @@ -61,6 +61,20 @@ void set_alloc_exit_log_func(exit_log_func_t func)
>  /* this magic number is 3671129837 decimal (623837458 complemented) */
>  #define LEAK_MAGIC 0xDAD0FEEDul
> 
> +#define INVALID_MAGIC 0xFE
> +#if __SIZEOF_POINTER__ == 4
> +#define INVALID_POINTER ((void*)0xFEFEFEFEul)
> +#endif
> +#if __SIZEOF_POINTER__ == 8
> +#define INVALID_POINTER ((void*)0xFEFEFEFEFEFEFEFEull)
> +#endif
> +#ifndef INVALID_POINTER
> +/* suppresses check */
> +#define INVALID_POINTER NULL
> +#endif
> +
> +static bool check_invalid_pointer = true;
> +
>  union mhdr {
>      struct {
>          const char *name;
> @@ -116,6 +130,9 @@ static void *alloc_bytes_raw(size_t size, const char
> *name)
>              allocs = p;
>              pthread_mutex_unlock(&leak_detective_mutex);
>          }
> +        if (p + 1 == INVALID_POINTER) {
> +            check_invalid_pointer = false; /* sigh! */
> +        }
>          return p + 1;
>      } else {
>          return p;
> @@ -128,8 +145,20 @@ void pfree(void *ptr)
>          union mhdr *p;
> 
>          passert(ptr != NULL);
> +        if (check_invalid_pointer) {
> +            if (ptr == INVALID_POINTER) {
> +                PASSERT_FAIL("pointer %p invalid (likely from use after
> free)", p);
> +            }
> +        }
> +
>          p = ((union mhdr *)ptr) - 1;
> -        passert(p->i.magic == LEAK_MAGIC);
> +
> +        if (p->i.magic == ~LEAK_MAGIC) {
> +            PASSERT_FAIL("pointer %p invalid (likely from double free)",
> ptr);
> +        } else if (p->i.magic != LEAK_MAGIC) {
> +            PASSERT_FAIL("pointer %p invalid (likely from heap
> corruption)", ptr);
> +        }
> +
>          {
>              pthread_mutex_lock(&leak_detective_mutex);
>              if (p->i.older != NULL) {
> @@ -146,7 +175,7 @@ void pfree(void *ptr)
>              pthread_mutex_unlock(&leak_detective_mutex);
>          }
>          /* stomp on memory!   Is another byte value better? */
> -        memset(p, 0xEF, sizeof(union mhdr) + p->i.size);
> +        memset(p, INVALID_MAGIC, sizeof(union mhdr) + p->i.size);
>          p->i.magic = ~LEAK_MAGIC;
>          free(p);
>      } else {

> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev