[PATCH] Erase memory before freeing.

[Justus Winter]

Hi :)

Ben Kibbey <bjk at luxsci.net> writes:

> * src/mem.h: New.

Looks like you forgot to add this file.

> * src/assuan-support.c (mem_item_s): New.
> (_gpgme_assuan_malloc_hooks): Replace to use tracking functions.
> (_gpgme_assuan_malloc): New.
> (_gpgme_assuan_realloc): Ditto.
> (_gpgme_assuan_free): Ditto.
> (free_locked): Ditto.
> (find_memlist_item): Ditto.
> * src/data-compat.c (gpgme_data_new_from_filepart): Wipe memory.
> * src/data-identify.c (gpgme_data_identify): Ditto.
> * src/data.c (_gpgme_data_release): Ditto.
>
> Signed-off-by: Ben Kibbey <bjk at luxsci.net>
> ---
>  src/assuan-support.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  src/data-compat.c    |  11 +++-
>  src/data-identify.c  |   3 ++
>  src/data-mem.c       |   6 ++-
>  src/data.c           |   5 ++
>  5 files changed, 160 insertions(+), 6 deletions(-)

[...]

> +static struct mem_item_s *memlist;

I wonder if a list is the right data structure.  Linear lookup time
feels expensive.

[...]

> +static void *
> +_gpgme_assuan_malloc (size_t n)
> +{
> +  struct mem_item_s *item;
> +  void *p;
> +
> +  p = malloc (n);
> +  if (!p)
> +    return NULL;
> +
> +  item = malloc (sizeof (struct mem_item_s));

I'd suggest to allocate both in one chunk.  That makes the error handling
simpler, decreases pressure on the allocator and increases locality.

> +  if (!item)
> +    {
> +      int e = errno;
> +      free (p);
> +      errno = e;
> +      return NULL;
> +    }
> +
> +  item->data = p;
> +  item->size = n;
> +  LOCK (memlist_lock);
> +  item->next = memlist;
> +  memlist = item;
> +  UNLOCK (memlist_lock);
> +  return item->data;
> +}
> +
> +
> +static void *
> +_gpgme_assuan_realloc (void *p, size_t n)
> +{
> +  struct mem_item_s *item;
> +  struct mem_item_s *newitem;
> +  void *tmp;
> +
> +  LOCK (memlist_lock);
> +  item = find_memlist_item (p, 0);
> +  if (!item)

I'm confused, shouldn't that be

if (p == NULL) return _gpgme_assuan_malloc (n);
item = find_memlist_item (p, 0);
assert (item);

instead ?

> +    {
> +      UNLOCK (memlist_lock);
> +      return _gpgme_assuan_malloc (n);
> +    }
> +
> +  tmp = _gpgme_assuan_malloc (n);

Can't we just use realloc here?  And if not for some reason, please
don't look newitem up like this, instead create a variant of
_gpgme_assuan_malloc that returns the item.

> +  if (tmp)
> +    newitem = find_memlist_item (tmp, 0);
> +  else
> +    {
> +      int e = errno;
> +      UNLOCK (memlist_lock);
> +      errno = e;
> +      return NULL;
> +    }
> +
> +  assert (newitem);
> +  memcpy (newitem->data, item->data,
> +          newitem->size < item->size ? newitem->size : item->size);
> +  free_locked (item->data, 1);
> +  UNLOCK (memlist_lock);
> +  return newitem->data;
> +}

Cheers,
Justus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 472 bytes
Desc: not available
URL: </pipermail/attachments/20160801/672d6a8f/attachment.sig>