Hi,
I thought of adding valgrind client requests to ClamAV's, but I
encountered some problems. I think it could be solved by introducing a
new client request in Valgrind.
While it is possible to teach valgrind about ClamAV's pools (see
attached mpool.patch for example), I think valgrind will be more
effective if I just disable ClamAV's pools (and let it use malloc).
The problem is that ClamAV's allocator doesn't have redzones, and
sometimes it allocates more memory than strictly requested.
All this makes it hard for valgrind to detect off-by-one errors, even
if I teach it about the pools.
Here is a rough idea of how ClamAV's pools look like:
mmap chunk_0 = | pool header | allocatable space |
mmap chunk_1 .. chunk_N = | block header | allocatable space |
Each allocation block looks like this:
| padding1| information about allocation | allocated data | padding2 |
Freed data is reusable, so freed blocks look like:
| ptr next free block | undefined data |
Also reallocs will reuse the allocation block
(when shrinking).
What I did was mark the entire allocation block with
VALGRIND_MEMPOOL_ALLOC, however this has several shortcomings.
1. Code can corrupt the allocation information (located just before the
allocated data), I could fix this by marking them as NOACCESS (ideal
would be readonly but valgrind doesn't have that notion).
2. If I fix 1), then I get warnings in the allocator itself (when it
wants to reuse freed data, or when it wants to realloc), I can fix by
marking the memory zones I want to access as UNDEFINED, however this
leads to problem 3
3. If I get passed an invalid pointer (one that the pool allocator
didn't allocate for example), then if I mark my allocation information
block as UNDEFINED I actually mark random memory and valgrind can't
help me
4. Realloc doesn't know the size of original allocation, it only knows
size of original allocation + padding2. So again imprecision and lack
of redzone at end of allocation. Realloc sometimes needs to memcpy() so
that would need me to temporarely mark the data as accessible
All this is complicated and error prone (I might call valgrind with the
wrong information).
I think a simple client request addition would solve this:
VALGRIND_MEMPOOL_ALLOCATIONSIZE(pool, addr) -> size of allocation if
addr was declared with VALGRIND_MEMPOOL_ALLOC(pool, addr, size), or 0
if addr belongs to a different pool, or no pool at all (it can be
noaccess/defined/undefined doesn't matter)
Then I could implement this in ClamAV like so:
- mark the paddings and allocator's allocation information as NOACCESS
after I fill it, and mark as allocated just exactly what
my mpool_malloc() call requested
- when I free/realloc I query valgrind whether the pointer is an
allocation base for the specified pool. If it is, I temporarely mark
the private allocation information readable and defined, I
read/modify it, then mark it as NOACCESS again. For realloc I would
know the exact allocation size and I would memcpy() only up to that
amount
Actually I could implement this already by abusing the meaning of
MPOOL_CREATE.
I could pretend each allocation is a pool in itself, mark only the
real allocation with MEMPOOL_ALLOC(), and use MEMPOOL_EXISTS() to check
whether the pointer really belongs to my pool, and is the start of an
allocation. However this might create millions of pool, which I guess
is not good for valgrind's performance.
Thoughts?
Best regards,
--Edwin
diff --git a/libclamav/mpool.c b/libclamav/mpool.c
index f81d51b..365e997 100644
--- a/libclamav/mpool.c
+++ b/libclamav/mpool.c
@@ -61,6 +61,7 @@ static inline void spam(const char *fmt, ...) { fmt = fmt; } /* gcc STFU */
#endif
#include "mpool.h"
+#include "memcheck.h"
#define MIN_FRAGSIZE 262144
@@ -344,12 +345,6 @@ struct MP *mpool_create() {
sz = align_to_pagesize(&mp, MIN_FRAGSIZE);
mp.u.mpm.usize = sizeof(struct MPMAP);
mp.u.mpm.size = sz - sizeof(mp);
-#ifndef _WIN32
- if ((mpool_p = (struct MP *)mmap(NULL, sz, PROT_READ | PROT_WRITE, MAP_PRIVATE|ANONYMOUS_MAP, -1, 0)) == MAP_FAILED)
-#else
- if(!(mpool_p = (struct MP *)VirtualAlloc(NULL, sz, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE)))
-#endif
- return NULL;
if (FRAGSBITS > 255) {
cli_errmsg("At most 255 frags possible!\n");
return NULL;
@@ -358,10 +353,18 @@ struct MP *mpool_create() {
cli_errmsg("fragsz[0] too small!\n");
return NULL;
}
+#ifndef _WIN32
+ if ((mpool_p = (struct MP *)mmap(NULL, sz, PROT_READ | PROT_WRITE, MAP_PRIVATE|ANONYMOUS_MAP, -1, 0)) == MAP_FAILED)
+#else
+ if(!(mpool_p = (struct MP *)VirtualAlloc(NULL, sz, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE)))
+#endif
+ return NULL;
#ifdef CL_DEBUG
memset(mpool_p, ALLOCPOISON, sz);
#endif
memcpy(mpool_p, &mp, sizeof(mp));
+ VALGRIND_CREATE_MEMPOOL(mpool_p, 0, 0);
+ VALGRIND_MAKE_MEM_NOACCESS(mpool_p+1, sz - sizeof(mp));
spam("Map created @%p->%p - size %u out of %u - voidptr=%u\n", mpool_p, (char *)mpool_p + mp.u.mpm.size, mp.u.mpm.usize, mp.u.mpm.size, SIZEOF_VOID_P);
return mpool_p;
}
@@ -373,9 +376,7 @@ void mpool_destroy(struct MP *mp) {
while((mpm = mpm_next)) {
mpmsize = mpm->size;
mpm_next = mpm->next;
-#ifdef CL_DEBUG
- memset(mpm, FREEPOISON, mpmsize);
-#endif
+ VALGRIND_MAKE_MEM_NOACCESS(mpm, mpmsize);
#ifndef _WIN32
munmap((void *)mpm, mpmsize);
#else
@@ -383,9 +384,7 @@ void mpool_destroy(struct MP *mp) {
#endif
}
mpmsize = mp->u.mpm.size;
-#ifdef CL_DEBUG
- memset(mp, FREEPOISON, mpmsize + sizeof(*mp));
-#endif
+ VALGRIND_MAKE_MEM_NOACCESS(mp, mpmsize + sizeof(*mp));
#ifndef _WIN32
munmap((void *)mp, mpmsize + sizeof(*mp));
#else
@@ -406,9 +405,7 @@ void mpool_flush(struct MP *mp) {
mpm_next = mpm->next;
mused = align_to_pagesize(mp, mpm->usize);
if(mused < mpm->size) {
-#ifdef CL_DEBUG
- memset((char *)mpm + mused, FREEPOISON, mpm->size - mused);
-#endif
+ VALGRIND_MAKE_MEM_NOACCESS((char*)mpm + mused, mpm->size - mused);
#ifndef _WIN32
munmap((char *)mpm + mused, mpm->size - mused);
#else
@@ -421,9 +418,7 @@ void mpool_flush(struct MP *mp) {
mused = align_to_pagesize(mp, mp->u.mpm.usize + sizeof(*mp));
if (mused < mp->u.mpm.size + sizeof(*mp)) {
-#ifdef CL_DEBUG
- memset((char *)mp + mused, FREEPOISON, mp->u.mpm.size + sizeof(*mp) - mused);
-#endif
+ VALGRIND_MAKE_MEM_NOACCESS((char*)mp + mused, mp->u.mpm.size + sizeof(*mp) - mused);
#ifndef _WIN32
munmap((char *)mp + mused, mp->u.mpm.size + sizeof(*mp) - mused);
#else
@@ -440,7 +435,7 @@ int mpool_getstats(const struct cl_engine *eng, size_t *used, size_t *total)
size_t sum_used = 0, sum_total = 0;
const struct MPMAP *mpm;
const mpool_t *mp;
-
+
/* checking refcount is not necessary, but safer */
if (!eng || !eng->refcount)
return -1;
@@ -462,7 +457,7 @@ static inline unsigned align_increase(unsigned size, unsigned a)
return size + a - 1;
}
-static void* allocate_aligned(struct MPMAP *mpm, unsigned long size, unsigned align, const char *dbg)
+static void* allocate_aligned(struct MP* mp, struct MPMAP *mpm, unsigned long size, unsigned align, const char *dbg)
{
/* We could always align the size to maxalign (8), however that wastes
* space.
@@ -480,6 +475,8 @@ static void* allocate_aligned(struct MPMAP *mpm, unsigned long size, unsigned al
#ifdef CL_DEBUG
assert(p_aligned + size <= mpm->size);
#endif
+
+ VALGRIND_MEMPOOL_ALLOC(mp, f, needed - (p_aligned - p));
f->u.a.sbits = sbits;
f->u.a.padding = p_aligned - p;
@@ -492,6 +489,7 @@ static void* allocate_aligned(struct MPMAP *mpm, unsigned long size, unsigned al
f->magic = MPOOLMAGIC;
memset(&f->u.a.fake, ALLOCPOISON, size);
#endif
+ VALGRIND_CHECK_MEM_IS_ADDRESSABLE(&f->u.a.fake, size);
return &f->u.a.fake;
}
@@ -514,6 +512,7 @@ void *mpool_malloc(struct MP *mp, size_t size) {
mp->avail[sbits] = f->u.next.ptr;
/* we always have enough space for this, align_increase ensured that */
f = (struct FRAG*)(alignto((unsigned long)f + FRAG_OVERHEAD, align)-FRAG_OVERHEAD);
+ VALGRIND_MEMPOOL_ALLOC(mp, f, from_bits(sbits) - ((char*)f - (char*)fold));
f->u.a.sbits = sbits;
f->u.a.padding = (char*)f - (char*)fold;
#ifdef CL_DEBUG
@@ -523,7 +522,6 @@ void *mpool_malloc(struct MP *mp, size_t size) {
spam("malloc @%p size %u (freed) origsize %u overhead %u\n", f, f->u.a.padding + FRAG_OVERHEAD + size, size, needed - size);
return &f->u.a.fake;
}
-
if (!(needed = from_bits(sbits))) {
cli_errmsg("mpool_malloc(): Attempt to allocate %lu bytes. Please report to http://bugs.clamav.net\n";, (unsigned long int) size);
return NULL;
@@ -532,7 +530,7 @@ void *mpool_malloc(struct MP *mp, size_t size) {
/* Case 2: We have nuff room available for this frag already */
while(mpm) {
if(mpm->size - mpm->usize >= needed)
- return allocate_aligned(mpm, size, align, "hole");
+ return allocate_aligned(mp, mpm, size, align, "hole");
mpm = mpm->next;
}
@@ -558,15 +556,20 @@ void *mpool_malloc(struct MP *mp, size_t size) {
mpm->usize = sizeof(*mpm);
mpm->next = mp->u.mpm.next;
mp->u.mpm.next = mpm;
- return allocate_aligned(mpm, size, align, "new map");
+ VALGRIND_MAKE_MEM_NOACCESS(mpm+1, mpm->size - mpm->usize);
+ return allocate_aligned(mp, mpm, size, align, "new map");
}
-static void *allocbase_fromfrag(struct FRAG *f)
+static void *allocbase_fromfrag(struct MP *mp, struct FRAG *f)
{
+ char *result;
#ifdef CL_DEBUG
assert(f->u.a.padding < 8);
#endif
- return (char*)f - f->u.a.padding;
+ result = (char*)f - f->u.a.padding;
+ VALGRIND_CHECK_MEM_IS_ADDRESSABLE(f, from_bits(f->u.a.sbits) - f->u.a.padding);
+ VALGRIND_MEMPOOL_FREE(mp, f);
+ return result;
}
void mpool_free(struct MP *mp, void *ptr) {
@@ -580,11 +583,13 @@ void mpool_free(struct MP *mp, void *ptr) {
spam("free @%p\n", f);
sbits = f->u.a.sbits;
- f = allocbase_fromfrag(f);
+ f = allocbase_fromfrag(mp, f);
#ifdef CL_DEBUG
+ VALGRIND_MAKE_MEM_UNDEFINED(f, from_bits(sbits));
memset(f, FREEPOISON, from_bits(sbits));
+ VALGRIND_MAKE_MEM_UNDEFINED(f, from_bits(sbits));
#endif
-
+ VALGRIND_MAKE_MEM_UNDEFINED(f, sizeof(*f));
f->u.next.ptr = mp->avail[sbits];
mp->avail[sbits] = f;
}
@@ -613,10 +618,12 @@ void *mpool_realloc(struct MP *mp, void *ptr, size_t size) {
if (csize >= size && (!f->u.a.sbits || from_bits(f->u.a.sbits-1)-FRAG_OVERHEAD-f->u.a.padding < size)) {
spam("free @%p\n", f);
spam("malloc @%p size %u (self) origsize %u overhead %u\n", f, size + FRAG_OVERHEAD + f->u.a.padding, size, csize-size+FRAG_OVERHEAD+f->u.a.padding);
+ VALGRIND_MEMPOOL_CHANGE(mp, f, f, size + FRAG_OVERHEAD);
return ptr;
}
if (!(new_ptr = mpool_malloc(mp, size)))
return NULL;
+ VALGRIND_CHECK_MEM_IS_ADDRESSABLE(ptr, csize);
memcpy(new_ptr, ptr, csize <= size ? csize : size);
mpool_free(mp, ptr);
return new_ptr;
------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
_______________________________________________
Valgrind-users mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/valgrind-users
