From 61a32cf95e9712e8d84e85c9cc0f057376009bd2 Mon Sep 17 00:00:00 2001 From: Tobias Frost Date: Wed, 22 Jun 2022 12:58:56 +0200 Subject: [PATCH 1/6] Consider page size in report_mmap / report_unmap. mmaps are full of quirks ;-) - mmap(2) will always allocate whole memory pages, even if the caller only reuqests a partial page. This is considered by calculating the "real" size of the mmap. - munmap(2) also operates on pages, unmapping every page it "touches.", so the size parameter is adjusted if needed. --- report.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/report.c b/report.c index cf3be27..6306b37 100644 --- a/report.c +++ b/report.c @@ -241,6 +241,9 @@ static void _report_calloc(struct task *task, struct library_symbol *libsym) report_alloc(task, MT_MALLOC, ret, size, options.bt_depth, libsym); } + +static ssize_t arch_pagesize = -1; + static void _report_mmap(struct task *task, struct library_symbol *libsym) { unsigned long ret = fetch_retval(task); @@ -249,6 +252,11 @@ static void _report_mmap(struct task *task, struct library_symbol *libsym) return; unsigned long size = fetch_param(task, 1); + if (unlikely(arch_pagesize==-1)) arch_pagesize=getpagesize(); + // fixup size, if size is not a multiple of the pagesize, we get the "partial" page too. - + if (size % arch_pagesize) { + size += arch_pagesize - size % arch_pagesize; + } report_alloc(task, MT_MMAP, ret, size, options.bt_depth, libsym); } @@ -277,6 +285,12 @@ static void _report_mmap64(struct task *task, struct library_symbol *libsym) else size.l = fetch_param(task, 1); + if (unlikely(arch_pagesize == -1)) arch_pagesize=getpagesize(); + // fixup size, if size is not a multiple of the pagesize, we get the "partial" page too. - + if (size.l % arch_pagesize) { + size.l += arch_pagesize - size.l % arch_pagesize; + } + report_alloc(task, MT_MMAP64, ret, size.l, options.bt_depth, libsym); } @@ -284,6 +298,12 @@ static void report_munmap(struct task *task, struct library_symbol *libsym) { unsigned long addr = fetch_param(task, 0); unsigned long size = fetch_param(task, 1); + if (unlikely(arch_pagesize==-1)) arch_pagesize=getpagesize(); + + // fixup size, if needed: all pages in [addr, addr+size] are unmapped -- see munmap(2) + if (size % arch_pagesize) { + size += arch_pagesize - size % arch_pagesize; + } report_alloc(task, MT_MUNMAP, addr, size, 0, libsym); } From f090e95e8379ab72611f70f1021e1afc88f8ae68 Mon Sep 17 00:00:00 2001 From: Tobias Frost Date: Wed, 22 Jun 2022 15:15:14 +0200 Subject: [PATCH 2/6] munmap can fail. According to munmap(2), the call can fail, e.g if the adress given is not at a page boundary. --- report.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/report.c b/report.c index 6306b37..e0528a8 100644 --- a/report.c +++ b/report.c @@ -294,20 +294,25 @@ static void _report_mmap64(struct task *task, struct library_symbol *libsym) report_alloc(task, MT_MMAP64, ret, size.l, options.bt_depth, libsym); } -static void report_munmap(struct task *task, struct library_symbol *libsym) +static void _report_munmap(struct task *task, struct library_symbol *libsym) { unsigned long addr = fetch_param(task, 0); unsigned long size = fetch_param(task, 1); - if (unlikely(arch_pagesize==-1)) arch_pagesize=getpagesize(); + unsigned long ret = fetch_retval(task); + + if(ret != 0 ) return; + + if(unlikely(arch_pagesize==-1)) arch_pagesize=getpagesize(); // fixup size, if needed: all pages in [addr, addr+size] are unmapped -- see munmap(2) - if (size % arch_pagesize) { + if(size % arch_pagesize) { size += arch_pagesize - size % arch_pagesize; } report_alloc(task, MT_MUNMAP, addr, size, 0, libsym); } + static void _report_memalign(struct task *task, struct library_symbol *libsym) { unsigned long size = fetch_param(task, 1); @@ -388,7 +393,7 @@ static const struct function flist[] = { { "posix_memalign", "posix_memalign", 0, NULL, _report_posix_memalign }, { "mmap", "mmap", 0, NULL, _report_mmap }, { "mmap64", "mmap64", 1, NULL, _report_mmap64 }, - { "munmap", "munmap", 0, report_munmap, NULL }, + { "munmap", "munmap", 0, NULL, _report_munmap }, { "memalign", "memalign", 0, NULL, _report_memalign }, { "aligned_alloc", "aligned_alloc", 1, NULL, _report_aligned_alloc }, { "valloc", "valloc", 1, NULL, _report_valloc }, From 871a79f35ba89030c31e12f49ad9872750f27429 Mon Sep 17 00:00:00 2001 From: Tobias Frost Date: Wed, 22 Jun 2022 15:17:48 +0200 Subject: [PATCH 3/6] mremap handling in case of error and creating a duplicate mapping. - mremap can fail, in this case the old mapping is retained. - mremap, when oldsize is 0, a new mapping is created without freeing the old one. See mremap(2) for details. --- report.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/report.c b/report.c index e0528a8..88d2895 100644 --- a/report.c +++ b/report.c @@ -369,20 +369,20 @@ static void _report_pvalloc(struct task *task, struct library_symbol *libsym) return report_alloc(task, MT_PVALLOC, ret, size, options.bt_depth, libsym); } -static void report_mremap(struct task *task, struct library_symbol *libsym) +static void _report_mremap(struct task *task, struct library_symbol *libsym) { unsigned long addr = fetch_param(task, 0); - unsigned long size = fetch_param(task, 1); - - report_alloc(task, MT_MUNMAP, addr, size, 0, libsym); -} + unsigned long oldsize = fetch_param(task, 1); -static void _report_mremap(struct task *task, struct library_symbol *libsym) -{ - unsigned long size = fetch_param(task, 2); + unsigned long newsize = fetch_param(task, 2); unsigned long ret = fetch_retval(task); - report_alloc(task, MT_MMAP, ret, size, options.bt_depth, libsym); + if( (void*)ret != MAP_FAILED) { + // mremap(2): if oldsize is zero and the mapping a shared mapping, a new mapping + // (Of the existing) will be created. + if (oldsize) report_alloc(task, MT_MUNMAP, addr, oldsize, 0, libsym); + report_alloc(task, MT_MMAP, ret, newsize, options.bt_depth, libsym); + } } static const struct function flist[] = { @@ -398,7 +398,7 @@ static const struct function flist[] = { { "aligned_alloc", "aligned_alloc", 1, NULL, _report_aligned_alloc }, { "valloc", "valloc", 1, NULL, _report_valloc }, { "pvalloc", "pvalloc", 1, NULL, _report_pvalloc }, - { "mremap", "mremap", 0, report_mremap, _report_mremap }, + { "mremap", "mremap", 0, NULL, _report_mremap }, { "cfree", "cfree", 1, report_free, NULL }, { "reallocarray", "reallocarray", 0, NULL, _report_reallocarray }, #if 0 From be3b4c22aeab21b6622361440e8c381f533aa03b Mon Sep 17 00:00:00 2001 From: Tobias Frost Date: Thu, 23 Jun 2022 09:18:43 +0200 Subject: [PATCH 4/6] Fix logic error in process_rb_search_range() The function would not properly check if addr is within the block due to inversed logic in the comparasion. --- client/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/process.c b/client/process.c index 8237fad..8e9b356 100644 --- a/client/process.c +++ b/client/process.c @@ -604,7 +604,7 @@ static struct rb_block *process_rb_search_range(struct rb_root *root, unsigned l while (node) { struct rb_block *this = container_of(node, struct rb_block, node); - if ((addr <= this->addr) && (addr + size > this->addr)) + if ((this->addr <= addr) && (this->addr + this->size > addr)) return this; if (addr < this->addr) From 3afc8f0a3f2aeef8183f634d0e9d92fd1097508c Mon Sep 17 00:00:00 2001 From: Tobias Frost Date: Thu, 23 Jun 2022 09:23:44 +0200 Subject: [PATCH 5/6] Fix process_munmap not handling munmap correctly. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit munmap(2) will unmap any page "it touches", the parameters given to it do not necessarily need to match the ones given to mmap(2). It is legit to specify pages that are not mapped at all, ranges that span multiple mappings, with or without holes… Beside that, a logic error in calculating the size for process_release_mem() is fixed in the case a munmap would split a previous allocation in two maps. Similar fix also for the case where the freed page is at the end of the allocated area. --- client/process.c | 51 +++++++++++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/client/process.c b/client/process.c index 8e9b356..18b2665 100644 --- a/client/process.c +++ b/client/process.c @@ -1298,20 +1298,32 @@ void process_munmap(struct process *process, struct mt_msg *mt_msg, void *payloa do { block = process_rb_search_range(&process->block_table, ptr, size); - if (!block) - break; - if (!is_mmap(block->stack_node->stack->operation)) { - if (unlikely(options.kill)) { - fprintf(stderr, ">>> block missmatch pid:%d MAP<>MALLOC %#lx\n", process->pid, ptr); - abort(); - } + if (block && !is_mmap(block->stack_node->stack->operation)) { + // ignore blocks not mmap'ed + block = NULL; + } - break; + if (!block) { + // printf("## no block found for %lx, %ld. Trying next page\n", ptr, size); + // it is legal to unmap arbitrary addresses, so ptr might be actually before the mmap and it might span muplitple mmaps areas. + // so we'll might need to search our blocks. + // eg this is legal: void* p=mmap(0,512*1024, PROT_WRITE|PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); munmap(p+4096,4096); munmap(p-4096, 512*1024+4096); + // FIXME pagesize is hardcoded as 4096 bytes, which should be safe (AFAIK 4k is the minimum "out there".) A more efficient way would be to transmit + // the target's PAGE_SIZE on startup. + if (size > 4096) { + size -= 4096; + ptr += 4096; + continue; + } + else { + break; + } } + // ptr in block -- this is already checked. if (block->addr >= ptr) { - unsigned off = block->addr - ptr; + unsigned long off = block->addr - ptr; size -= off; ptr += off; @@ -1331,33 +1343,32 @@ void process_munmap(struct process *process, struct mt_msg *mt_msg, void *payloa process_rb_delete_block(process, block); } else { - unsigned off = ptr - block->addr; + unsigned long off = ptr - block->addr; if (off + size < block->size) { unsigned long new_addr = block->addr + (off + size); unsigned long new_size = block->size - (off + size); - process_release_mem(process, block, block->size - off - new_size); + process_release_mem(process, block, block->size - off); block->size = off; if (process_rb_insert_block(process, new_addr, new_size, block->stack_node, 0, mt_msg->operation)) break; - process->n_allocations++; process->total_allocations++; process->bytes_used += new_size; - break; } + else { + // freeing a chunk at the end of the mmap block. + size_t amount_freed = block->size - off; + process_release_mem(process, block, amount_freed); - process_release_mem(process, block, off); - - block->addr += off; - block->size -= off; - - size -= block->size; - ptr += block->size; + block->size -= amount_freed; + size -= amount_freed ; + ptr += amount_freed; + } } } while(size); } From 003dd834fda05dadb7645bc9b00a221a659ddb93 Mon Sep 17 00:00:00 2001 From: Tobias Frost Date: Fri, 24 Jun 2022 11:14:43 +0200 Subject: [PATCH 6/6] Consistently use long for the pid. --- client/process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/process.c b/client/process.c index 18b2665..d032b87 100644 --- a/client/process.c +++ b/client/process.c @@ -1500,7 +1500,7 @@ void process_free(struct process *process, struct mt_msg *mt_msg, void *payload) void process_realloc_done(struct process *process, struct mt_msg *mt_msg, void *payload) { unsigned long ptr; - unsigned int pid; + unsigned long pid; struct list_head *it; (void)mt_msg; @@ -1536,7 +1536,7 @@ void process_realloc_done(struct process *process, struct mt_msg *mt_msg, void * } if (unlikely(options.kill)) { - fprintf(stderr, ">>> unexpected realloc done pid: %u ptr: %#lx\n", pid, ptr); + fprintf(stderr, ">>> unexpected realloc done pid: %lu ptr: %#lx\n", pid, ptr); abort(); } return;