From 5c866e44376ab5dcb4fcfc2725cdd6d47168236a Mon Sep 17 00:00:00 2001 From: Arnaud Giersch Date: Thu, 24 Jun 2021 11:10:06 +0200 Subject: [PATCH] Parameter 'fd' is always -1 for xbt_mheap_new. Kill dead code. --- src/include/xbt/mmalloc.h | 4 +- src/xbt/mmalloc/mm_module.c | 142 ++----------------------- src/xbt/mmalloc/mmorecore.c | 116 ++++++-------------- src/xbt/mmalloc/mmprivate.h | 17 +-- teshsuite/xbt/mmalloc/mmalloc_test.cpp | 2 +- 5 files changed, 44 insertions(+), 237 deletions(-) diff --git a/src/include/xbt/mmalloc.h b/src/include/xbt/mmalloc.h index bd70114f47..33235944a6 100644 --- a/src/include/xbt/mmalloc.h +++ b/src/include/xbt/mmalloc.h @@ -41,11 +41,9 @@ XBT_PUBLIC void* mrealloc(xbt_mheap_t md, void* ptr, size_t size); /* Free a block allocated by `mmalloc', `mrealloc' or `mcalloc'. */ XBT_PUBLIC void mfree(xbt_mheap_t md, void* ptr); -XBT_PUBLIC xbt_mheap_t xbt_mheap_new(int fd, void* baseaddr); - #define XBT_MHEAP_OPTION_MEMSET 1 -XBT_PUBLIC xbt_mheap_t xbt_mheap_new_options(int fd, void* baseaddr, int options); +XBT_PUBLIC xbt_mheap_t xbt_mheap_new(void* baseaddr, int options); XBT_PUBLIC void xbt_mheap_destroy_no_free(xbt_mheap_t md); diff --git a/src/xbt/mmalloc/mm_module.c b/src/xbt/mmalloc/mm_module.c index cd4d7b70f7..5c95f1a427 100644 --- a/src/xbt/mmalloc/mm_module.c +++ b/src/xbt/mmalloc/mm_module.c @@ -32,35 +32,14 @@ #include /* After sys/types.h, at least for dpx/2. */ #include #include -#if HAVE_UNISTD_H -#include /* Prototypes for lseek */ -#endif #include "mmprivate.h" #include "xbt/ex.h" #include "xbt/xbt_modinter.h" /* declarations of mmalloc_preinit and friends that live here */ -#ifndef SEEK_SET -#define SEEK_SET 0 -#endif - /* Initialize access to a mmalloc managed region. - If FD is a valid file descriptor for an open file then data for the - mmalloc managed region is mapped to that file, otherwise an anonymous - map is used if supported by the underlying OS. In case of running in - an OS without support of anonymous mappings then "/dev/zero" is used - and in both cases the data will not exist in any filesystem object. - - If the open file corresponding to FD is from a previous use of - mmalloc and passes some basic sanity checks to ensure that it is - compatible with the current mmalloc package, then its data is - mapped in and is immediately accessible at the same addresses in - the current process as the process that created the file (ignoring - the BASEADDR parameter). - - For non valid FDs or empty files ones the mapping is established - starting at the specified address BASEADDR in the process address - space. + The mapping is established starting at the specified address BASEADDR + in the process address space. The provided BASEADDR should be chosen carefully in order to avoid bumping into existing mapped regions or future mapped regions. @@ -72,91 +51,8 @@ On failure returns NULL. */ -xbt_mheap_t xbt_mheap_new(int fd, void *baseaddr) -{ - return xbt_mheap_new_options(fd, baseaddr, 0); -} - -xbt_mheap_t xbt_mheap_new_options(int fd, void *baseaddr, int options) +xbt_mheap_t xbt_mheap_new(void* baseaddr, int options) { - struct mdesc mtemp; - xbt_mheap_t mdp; - void *mbase; - struct stat sbuf; - - /* First check to see if FD is a valid file descriptor, and if so, see - if the file has any current contents (size > 0). If it does, then - attempt to reuse the file. If we can't reuse the file, either - because it isn't a valid mmalloc produced file, was produced by an - obsolete version, or any other reason, then we fail to attach to - this file. */ - - if (fd >= 0) { - if (fstat(fd, &sbuf) < 0) - return NULL; - - else if (sbuf.st_size > 0) { - /* We were given a valid file descriptor on an open file, so try to remap - it into the current process at the same address to which it was previously - mapped. It naturally have to pass some sanity checks for that. - - Note that we have to update the file descriptor number in the malloc- - descriptor read from the file to match the current valid one, before - trying to map the file in, and again after a successful mapping and - after we've switched over to using the mapped in malloc descriptor - rather than the temporary one on the stack. - - Once we've switched over to using the mapped in malloc descriptor, we - have to update the pointer to the morecore function, since it almost - certainly will be at a different address if the process reusing the - mapped region is from a different executable. - - Also note that if the heap being remapped previously used the mmcheckf() - routines, we need to update the hooks since their target functions - will have certainly moved if the executable has changed in any way. - We do this by calling mmcheckf() internally. - - Returns a pointer to the malloc descriptor if successful, or NULL if - unsuccessful for some reason. */ - - struct mdesc newmd; - struct mdesc* mdptr = NULL; - struct mdesc* mdptemp = NULL; - - if (lseek(fd, 0L, SEEK_SET) != 0) - return NULL; - if (read(fd, (char *) &newmd, sizeof(newmd)) != sizeof(newmd)) - return NULL; - if (newmd.headersize != sizeof(newmd)) - return NULL; - if (strcmp(newmd.magic, MMALLOC_MAGIC) != 0) - return NULL; - if (newmd.version > MMALLOC_VERSION) - return NULL; - - newmd.fd = fd; - if (__mmalloc_remap_core(&newmd) == newmd.base) { - mdptr = (struct mdesc *) newmd.base; - mdptr->fd = fd; - if(!mdptr->refcount){ - pthread_mutex_init(&mdptr->mutex, NULL); - mdptr->refcount++; - } - } - - /* Add the new heap to the linked list of heaps attached by mmalloc */ - mdptemp = __mmalloc_default_mdp; - while(mdptemp->next_mdesc) - mdptemp = mdptemp->next_mdesc; - - LOCK(mdptemp); - mdptemp->next_mdesc = mdptr; - UNLOCK(mdptemp); - - return mdptr; - } - } - /* NULL is not a valid baseaddr as we cannot map anything there. C'mon, user. Think! */ if (baseaddr == NULL) return NULL; @@ -165,23 +61,16 @@ xbt_mheap_t xbt_mheap_new_options(int fd, void *baseaddr, int options) * call _mmalloc_mmap_morecore() to allocate the first page of the region and copy it there. Ensure that it is * zero'd and then initialize the fields that we know values for. */ - mdp = &mtemp; + struct mdesc mtemp; + xbt_mheap_t mdp = &mtemp; memset((char *) mdp, 0, sizeof(mtemp)); strncpy(mdp->magic, MMALLOC_MAGIC, MMALLOC_MAGIC_SIZE); mdp->headersize = sizeof(mtemp); mdp->version = MMALLOC_VERSION; - mdp->fd = fd; mdp->base = mdp->breakval = mdp->top = baseaddr; mdp->next_mdesc = NULL; - mdp->refcount = 1; mdp->options = options; - /* If we have not been passed a valid open file descriptor for the file - to map to, then we go for an anonymous map */ - - if (mdp->fd < 0){ - mdp->flags |= MMALLOC_ANONYMOUS; - } pthread_mutex_init(&mdp->mutex, NULL); /* If we have not been passed a valid open file descriptor for the file to map to, then open /dev/zero and use that to map to. */ @@ -190,12 +79,12 @@ xbt_mheap_t xbt_mheap_new_options(int fd, void *baseaddr, int options) * this new copy. If the mapping fails, then close the file descriptor if it was opened by us, and arrange to return * a NULL. */ - if ((mbase = mmorecore(mdp, sizeof(mtemp))) != NULL) { - memcpy(mbase, mdp, sizeof(mtemp)); - } else { + void* mbase = mmorecore(mdp, sizeof(mtemp)); + if (mbase == NULL) { fprintf(stderr, "morecore failed to get some more memory!\n"); abort(); } + memcpy(mbase, mdp, sizeof(mtemp)); /* Add the new heap to the linked list of heaps attached by mmalloc */ if(__mmalloc_default_mdp){ @@ -222,9 +111,7 @@ void xbt_mheap_destroy_no_free(xbt_mheap_t md) { struct mdesc *mdp = md; - if(--mdp->refcount == 0){ - pthread_mutex_destroy(&mdp->mutex); - } + pthread_mutex_destroy(&mdp->mutex); } /** Terminate access to a mmalloc managed region by unmapping all memory pages associated with the region, and closing @@ -258,9 +145,6 @@ void *xbt_mheap_destroy(xbt_mheap_t mdp) /* Deallocating failed. Update the original malloc descriptor with any changes */ *mdp = mtemp; } else { - if (mtemp.flags & MMALLOC_DEVZERO) { - close(mtemp.fd); - } mdp = NULL; } } @@ -278,9 +162,6 @@ static void mmalloc_fork_prepare(void) if ((mdp =__mmalloc_default_mdp)){ while(mdp){ LOCK(mdp); - if(mdp->fd >= 0){ - mdp->refcount++; - } mdp = mdp->next_mdesc; } } @@ -291,8 +172,7 @@ static void mmalloc_fork_parent(void) xbt_mheap_t mdp = NULL; if ((mdp =__mmalloc_default_mdp)){ while(mdp){ - if(mdp->fd < 0) - UNLOCK(mdp); + UNLOCK(mdp); mdp = mdp->next_mdesc; } } @@ -317,7 +197,7 @@ xbt_mheap_t mmalloc_preinit(void) xbt_pagesize = getpagesize(); unsigned long mask = ~((unsigned long)xbt_pagesize - 1); void *addr = (void*)(((unsigned long)sbrk(0) + HEAP_OFFSET) & mask); - __mmalloc_default_mdp = xbt_mheap_new_options(-1, addr, XBT_MHEAP_OPTION_MEMSET); + __mmalloc_default_mdp = xbt_mheap_new(addr, XBT_MHEAP_OPTION_MEMSET); // atfork mandated at least on FreeBSD, or simgrid-mc will fail to fork the verified app int res = pthread_atfork(mmalloc_fork_prepare, mmalloc_fork_parent, mmalloc_fork_child); diff --git a/src/xbt/mmalloc/mmorecore.c b/src/xbt/mmalloc/mmorecore.c index d7cd4e1974..1dff798c54 100644 --- a/src/xbt/mmalloc/mmorecore.c +++ b/src/xbt/mmalloc/mmorecore.c @@ -11,9 +11,6 @@ Contributed by Fred Fish at Cygnus Support. fnf@cygnus.com */ #include "src/internal_config.h" -#if HAVE_UNISTD_H -#include /* Prototypes for lseek */ -#endif #include #include #include @@ -29,27 +26,6 @@ #define PAGE_ALIGN(addr) (void*) (((long)(addr) + xbt_pagesize - 1) & \ ~((long)xbt_pagesize - 1)) -/* Return MAP_PRIVATE if MDP represents /dev/zero. Otherwise, return - MAP_SHARED. */ -#define MAP_PRIVATE_OR_SHARED(MDP) (( MDP -> flags & MMALLOC_ANONYMOUS) \ - ? MAP_PRIVATE \ - : MAP_SHARED) - -/* Return MAP_ANONYMOUS if MDP uses anonymous mapping. Otherwise, return 0 */ -#define MAP_IS_ANONYMOUS(MDP) (((MDP) -> flags & MMALLOC_ANONYMOUS) \ - ? MAP_ANONYMOUS \ - : 0) - -/* Return -1 if MDP uses anonymous mapping. Otherwise, return MDP->FD */ -#define MAP_ANON_OR_FD(MDP) (((MDP) -> flags & MMALLOC_ANONYMOUS) \ - ? -1 \ - : (MDP) -> fd) - -/* Return 0if MDP uses anonymous mapping. Otherwise, return off */ -#define MAP_ANON_OR_OFFSET(MDP, off) (((MDP) -> flags & MMALLOC_ANONYMOUS) \ - ? 0 \ - : off) - /** @brief Add memory to this heap * * Get core for the memory region specified by MDP, using SIZE as the @@ -64,11 +40,9 @@ void *mmorecore(struct mdesc *mdp, ssize_t size) { void* result; // please keep it uninitialized to track issues - off_t foffset; /* File offset at which new mapping will start */ size_t mapbytes; /* Number of bytes to map */ void* moveto; /* Address where we wish to move "break value" to */ void* mapto; /* Address we actually mapped to */ - char buf = 0; /* Single byte to write to extend mapped file */ if (size == 0) { /* Just return the current "break" value. */ @@ -88,69 +62,39 @@ void *mmorecore(struct mdesc *mdp, ssize_t size) fprintf(stderr,"Internal error: mmap was asked to deallocate more memory than it previously allocated. Bailling out now!\n"); abort(); } - } else { - /* We are allocating memory. Make sure we have an open file descriptor if not working with anonymous memory. */ - if (!(mdp->flags & MMALLOC_ANONYMOUS) && mdp->fd < 0) { - fprintf(stderr,"Internal error: mmap file descriptor <0 (%d), without MMALLOC_ANONYMOUS being in the flags.\n",mdp->fd); + } else if ((char*)mdp->breakval + size > (char*)mdp->top) { + /* The request would move us past the end of the currently mapped memory, so map in enough more memory to satisfy + the request. This means we also have to grow the mapped-to file by an appropriate amount, since mmap cannot + be used to extend a file. */ + moveto = PAGE_ALIGN((char*)mdp->breakval + size); + mapbytes = (char*)moveto - (char*)mdp->top; + + /* Let's call mmap. Note that it is possible that mdp->top is 0. In this case mmap will choose the address for us. + This call might very well overwrite an already existing memory mapping (leading to weird bugs). + */ + mapto = mmap(mdp->top, mapbytes, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); + + if (mapto == MAP_FAILED) { + char buff[1024]; + fprintf(stderr, "Internal error: mmap returned MAP_FAILED! error: %s\n", strerror(errno)); + snprintf(buff, 1024, "cat /proc/%d/maps", getpid()); + int status = system(buff); + if (status == -1 || !(WIFEXITED(status) && WEXITSTATUS(status) == 0)) + fprintf(stderr, "Something went wrong when trying to %s\n", buff); + sleep(1); abort(); - } else if ((char*)mdp->breakval + size > (char*)mdp->top) { - /* The request would move us past the end of the currently mapped memory, so map in enough more memory to satisfy - the request. This means we also have to grow the mapped-to file by an appropriate amount, since mmap cannot - be used to extend a file. */ - moveto = PAGE_ALIGN((char*)mdp->breakval + size); - mapbytes = (char*)moveto - (char*)mdp->top; - foffset = (char*)mdp->top - (char*)mdp->base; - - if (mdp->fd > 0) { - if (lseek(mdp->fd, foffset + mapbytes - 1, SEEK_SET) == -1) { - fprintf(stderr, "Internal error: lseek into mmap'ed fd failed! error: %s", strerror(errno)); - abort(); - } - if (write(mdp->fd, &buf, 1) == -1) { - fprintf(stderr,"Internal error: write to mmap'ed fd failed! error: %s", strerror(errno)); - abort(); - } - } - - /* Let's call mmap. Note that it is possible that mdp->top is 0. In this case mmap will choose the address for us. - This call might very well overwrite an already existing memory mapping (leading to weird bugs). - */ - mapto = mmap(mdp->top, mapbytes, PROT_READ | PROT_WRITE, - MAP_PRIVATE_OR_SHARED(mdp) | MAP_IS_ANONYMOUS(mdp) | - MAP_FIXED, MAP_ANON_OR_FD(mdp), MAP_ANON_OR_OFFSET(mdp, foffset)); - - if (mapto == MAP_FAILED) { - char buff[1024]; - fprintf(stderr,"Internal error: mmap returned MAP_FAILED! error: %s\n",strerror(errno)); - snprintf(buff,1024,"cat /proc/%d/maps",getpid()); - int status = system(buff); - if (status == -1 || !(WIFEXITED(status) && WEXITSTATUS(status) == 0)) - fprintf(stderr, "Something went wrong when trying to %s\n", buff); - sleep(1); - abort(); - } + } - if (mdp->top == 0) - mdp->base = mdp->breakval = mapto; + if (mdp->top == 0) + mdp->base = mdp->breakval = mapto; - mdp->top = PAGE_ALIGN((char*)mdp->breakval + size); - result = mdp->breakval; - mdp->breakval = (char*)mdp->breakval + size; - } else { - /* Memory is already mapped, we only need to increase the breakval: */ - result = mdp->breakval; - mdp->breakval = (char*)mdp->breakval + size; - } + mdp->top = PAGE_ALIGN((char*)mdp->breakval + size); + result = mdp->breakval; + mdp->breakval = (char*)mdp->breakval + size; + } else { + /* Memory is already mapped, we only need to increase the breakval: */ + result = mdp->breakval; + mdp->breakval = (char*)mdp->breakval + size; } return result; } - -void* __mmalloc_remap_core(const s_xbt_mheap_t* mdp) -{ - /* FIXME: Quick hack, needs error checking and other attention. */ - - return mmap(mdp->base, (char*) mdp->top - (char*) mdp->base, - PROT_READ | PROT_WRITE | PROT_EXEC, - MAP_PRIVATE_OR_SHARED(mdp) | MAP_FIXED, mdp->fd, 0); -} - diff --git a/src/xbt/mmalloc/mmprivate.h b/src/xbt/mmalloc/mmprivate.h index 9866448dc1..bca9acddeb 100644 --- a/src/xbt/mmalloc/mmprivate.h +++ b/src/xbt/mmalloc/mmprivate.h @@ -173,9 +173,6 @@ struct mdesc { /** @brief Mutex locking the access to the heap */ pthread_mutex_t mutex; - /** @brief Number of processes that attached the heap */ - unsigned int refcount; - /** @brief Chained lists of mdescs */ struct mdesc *next_mdesc; @@ -245,13 +242,6 @@ struct mdesc { */ void *top; - /** @brief Open file descriptor for the file to which this malloc heap is mapped - * - * If this value is negative, MAP_ANONYMOUS memory is used. - * - * Also note that it may change each time the region is mapped and unmapped. */ - int fd; - /* @brief Instrumentation */ struct mstats heapstats; }; @@ -259,17 +249,12 @@ struct mdesc { /* Bits to look at in the malloc descriptor flags word */ #define MMALLOC_DEVZERO (1 << 0) /* Have mapped to /dev/zero */ -#define MMALLOC_ANONYMOUS (1 << 1) /* Use anonymous mapping */ -#define MMALLOC_INITIALIZED (1 << 2) /* Initialized mmalloc */ +#define MMALLOC_INITIALIZED (1 << 1) /* Initialized mmalloc */ /* A default malloc descriptor for the single sbrk() managed region. */ XBT_PUBLIC_DATA struct mdesc* __mmalloc_default_mdp; -/* Remap a mmalloc region that was previously mapped. */ - -XBT_PUBLIC void* __mmalloc_remap_core(const s_xbt_mheap_t* mdp); - XBT_PUBLIC void* mmorecore(struct mdesc* mdp, ssize_t size); /** Thread-safety (if the mutex is already created) diff --git a/teshsuite/xbt/mmalloc/mmalloc_test.cpp b/teshsuite/xbt/mmalloc/mmalloc_test.cpp index 1a370eb196..76196b812c 100644 --- a/teshsuite/xbt/mmalloc/mmalloc_test.cpp +++ b/teshsuite/xbt/mmalloc/mmalloc_test.cpp @@ -39,7 +39,7 @@ int main(int argc, char**argv) XBT_INFO("Allocating a new heap"); unsigned long mask = ~((unsigned long)xbt_pagesize - 1); auto* addr = reinterpret_cast(((unsigned long)sbrk(0) + BUFFSIZE) & mask); - heapA = xbt_mheap_new(-1, addr); + heapA = xbt_mheap_new(addr, 0); if (heapA == nullptr) { perror("attach 1 failed"); fprintf(stderr, "bye\n"); -- 2.20.1