From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11431 invoked by alias); 21 Jun 2019 12:13:49 -0000 Mailing-List: contact gdb-cvs-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: , Sender: gdb-cvs-owner@sourceware.org List-Subscribe: Sender: gdb-cvs-owner@sourceware.org Received: (qmail 11398 invoked by uid 10018); 21 Jun 2019 12:13:49 -0000 Date: Fri, 21 Jun 2019 12:13:00 -0000 Message-ID: <20190621121349.11397.qmail@sourceware.org> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: Jose E.Marchesi To: bfd-cvs@sourceware.org, gdb-cvs@sourceware.org Subject: [binutils-gdb] libctf: drop mmap()-based CTF data allocator X-Act-Checkin: binutils-gdb X-Git-Author: Nick Alcock X-Git-Refname: refs/heads/master X-Git-Oldrev: 24865428034f44d9fffe6b2d9a318e1bd507c63a X-Git-Newrev: 65365aa856e5a258329dc350b02bbb51f84b4c16 X-SW-Source: 2019-06/txt/msg00118.txt.bz2 https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=65365aa856e5a258329dc350b02bbb51f84b4c16 commit 65365aa856e5a258329dc350b02bbb51f84b4c16 Author: Nick Alcock Date: Wed Jun 19 12:20:47 2019 +0100 libctf: drop mmap()-based CTF data allocator This allocator has the ostensible benefit that it lets us mprotect() the memory used for CTF storage: but in exchange for this it adds considerable complexity, since we have to track allocation sizes ourselves for use at freeing time, note whether the data we are storing was ctf_data_alloc()ed or not so we know if we can safely mprotect() it... and while the mprotect()ing has found few bugs, it *has* been the cause of more than one due to errors in all this tracking leading to us mprotect()ing bits of the heap and stuff like that. We are about to start composing CTF buffers from pieces so that we can do usage-based optimizations on the strtab. This means we need realloc(), which needs nonportable mremap() and *more* tracking of the *original* allocation size, and the complexity and bureaucracy of all of this is just too high for its negligible benefits. Drop the whole thing and just use malloc() like everyone else. It knows better than we do when it is safe to use mmap() under the covers, anyway. While we're at it, don't leak the entire buffer if ctf_compress_write() fails to compress it. libctf/ * ctf-subr.c (_PAGESIZE): Remove. (ctf_data_alloc): Likewise. (ctf_data_free): Likewise. (ctf_data_protect): Likewise. * ctf-impl.h: Remove declarations. * ctf-create.c (ctf_update): No longer call ctf_data_protect: use ctf_free, not ctf_data_free. (ctf_compress_write): Use ctf_data_alloc, not ctf_alloc. Free the buffer again on compression error. * ctf-open.c (ctf_set_base): No longer track the size: call ctf_free, not ctf_data_free. (upgrade_types): Likewise. Call ctf_alloc, not ctf_data_alloc. (ctf_bufopen): Likewise. No longer call ctf_data_protect. Diff: --- libctf/ChangeLog | 16 ++++++++++++++++ libctf/ctf-create.c | 15 +++++++-------- libctf/ctf-impl.h | 5 ----- libctf/ctf-open.c | 34 +++++++++------------------------- libctf/ctf-subr.c | 51 --------------------------------------------------- 5 files changed, 32 insertions(+), 89 deletions(-) diff --git a/libctf/ChangeLog b/libctf/ChangeLog index 886e30b..9fd9e66 100644 --- a/libctf/ChangeLog +++ b/libctf/ChangeLog @@ -1,5 +1,21 @@ 2019-06-19 Nick Alcock + * ctf-subr.c (_PAGESIZE): Remove. + (ctf_data_alloc): Likewise. + (ctf_data_free): Likewise. + (ctf_data_protect): Likewise. + * ctf-impl.h: Remove declarations. + * ctf-create.c (ctf_update): No longer call ctf_data_protect: use + ctf_free, not ctf_data_free. + (ctf_compress_write): Use ctf_data_alloc, not ctf_alloc. Free + the buffer again on compression error. + * ctf-open.c (ctf_set_base): No longer track the size: call + ctf_free, not ctf_data_free. + (upgrade_types): Likewise. Call ctf_alloc, not ctf_data_alloc. + (ctf_bufopen): Likewise. No longer call ctf_data_protect. + +2019-06-19 Nick Alcock + * ctf-create.c (ctf_dtd_insert): Pass on error returns from ctf_dynhash_insert. (ctf_dvd_insert): Likewise. diff --git a/libctf/ctf-create.c b/libctf/ctf-create.c index 5b53479..86695f5 100644 --- a/libctf/ctf-create.c +++ b/libctf/ctf-create.c @@ -310,7 +310,7 @@ ctf_update (ctf_file_t *fp) buf_size = sizeof (ctf_header_t) + hdr.cth_stroff + hdr.cth_strlen; - if ((buf = ctf_data_alloc (buf_size)) == NULL) + if ((buf = malloc (buf_size)) == NULL) return (ctf_set_errno (fp, EAGAIN)); memcpy (buf, &hdr, sizeof (ctf_header_t)); @@ -449,11 +449,9 @@ ctf_update (ctf_file_t *fp) /* Finally, we are ready to ctf_simple_open() the new container. If this is successful, we then switch nfp and fp and free the old container. */ - ctf_data_protect (buf, buf_size); - if ((nfp = ctf_simple_open (buf, buf_size, NULL, 0, 0, NULL, 0, &err)) == NULL) { - ctf_data_free (buf, buf_size); + ctf_free (buf); return (ctf_set_errno (fp, err)); } @@ -462,7 +460,7 @@ ctf_update (ctf_file_t *fp) nfp->ctf_refcnt = fp->ctf_refcnt; nfp->ctf_flags |= fp->ctf_flags & ~LCTF_DIRTY; - nfp->ctf_data.cts_data = NULL; /* Force ctf_data_free() on close. */ + nfp->ctf_data.cts_data = NULL; /* Force ctf_free() on close. */ nfp->ctf_dthash = fp->ctf_dthash; nfp->ctf_dtdefs = fp->ctf_dtdefs; nfp->ctf_dtbyname = fp->ctf_dtbyname; @@ -1993,16 +1991,17 @@ ctf_compress_write (ctf_file_t *fp, int fd) memcpy (hp, fp->ctf_base, header_len); hp->cth_flags |= CTF_F_COMPRESS; - if ((buf = ctf_data_alloc (max_compress_len)) == NULL) + if ((buf = ctf_alloc (max_compress_len)) == NULL) return (ctf_set_errno (fp, ECTF_ZALLOC)); compress_len = max_compress_len; - if ((rc = compress (buf, (uLongf *) & compress_len, + if ((rc = compress (buf, (uLongf *) &compress_len, fp->ctf_base + header_len, fp->ctf_size - header_len)) != Z_OK) { ctf_dprintf ("zlib deflate err: %s\n", zError (rc)); err = ctf_set_errno (fp, ECTF_COMPRESS); + ctf_free (buf); goto ret; } @@ -2030,7 +2029,7 @@ ctf_compress_write (ctf_file_t *fp, int fd) } ret: - ctf_data_free (buf, max_compress_len); + ctf_free (buf); return err; } diff --git a/libctf/ctf-impl.h b/libctf/ctf-impl.h index 52aef24..44f0493 100644 --- a/libctf/ctf-impl.h +++ b/libctf/ctf-impl.h @@ -343,11 +343,6 @@ extern void *ctf_set_open_errno (int *, int); extern unsigned long ctf_set_errno (ctf_file_t *, int); _libctf_malloc_ -extern void *ctf_data_alloc (size_t); -extern void ctf_data_free (void *, size_t); -extern void ctf_data_protect (void *, size_t); - -_libctf_malloc_ extern void *ctf_mmap (size_t length, size_t offset, int fd); extern void ctf_munmap (void *, size_t); extern ssize_t ctf_pread (int fd, void *buf, ssize_t count, off_t offset); diff --git a/libctf/ctf-open.c b/libctf/ctf-open.c index 1a517a1..b0d3ef6 100644 --- a/libctf/ctf-open.c +++ b/libctf/ctf-open.c @@ -345,24 +345,17 @@ ctf_set_base (ctf_file_t *fp, const ctf_header_t *hp, void *base) /* Free a ctf_base pointer: the pointer passed, or (if NULL) fp->ctf_base. */ static void -ctf_free_base (ctf_file_t *fp, unsigned char *ctf_base, size_t ctf_size) +ctf_free_base (ctf_file_t *fp, unsigned char *ctf_base) { unsigned char *base; - size_t size; if (ctf_base) - { base = ctf_base; - size = ctf_size; - } else - { base = (unsigned char *) fp->ctf_base; - size = fp->ctf_size; - } if (base != fp->ctf_data.cts_data && base != NULL) - ctf_data_free (base, size); + ctf_free (base); } /* Set the version of the CTF file. */ @@ -392,7 +385,6 @@ upgrade_types (ctf_file_t *fp, ctf_header_t *cth) const ctf_type_v1_t *tbuf; const ctf_type_v1_t *tend; unsigned char *ctf_base, *old_ctf_base = (unsigned char *) fp->ctf_base; - size_t old_ctf_size = fp->ctf_size; ctf_type_t *t2buf; ssize_t increase = 0, size, increment, v2increment, vbytes, v2bytes; @@ -439,7 +431,7 @@ upgrade_types (ctf_file_t *fp, ctf_header_t *cth) version number unchanged, so that LCTF_INFO_* still works on the as-yet-untranslated type info. */ - if ((ctf_base = ctf_data_alloc (fp->ctf_size + increase)) == NULL) + if ((ctf_base = ctf_alloc (fp->ctf_size + increase)) == NULL) return ECTF_ZALLOC; memcpy (ctf_base, fp->ctf_base, sizeof (ctf_header_t) + cth->cth_typeoff); @@ -608,7 +600,7 @@ upgrade_types (ctf_file_t *fp, ctf_header_t *cth) assert ((size_t) t2p - (size_t) fp->ctf_buf == new_cth->cth_stroff); ctf_set_version (fp, (ctf_header_t *) ctf_base, CTF_VERSION_1_UPGRADED_3); - ctf_free_base (fp, old_ctf_base, old_ctf_size); + ctf_free_base (fp, old_ctf_base); memcpy (cth, new_cth, sizeof (ctf_header_t)); return 0; @@ -1319,7 +1311,7 @@ ctf_bufopen (const ctf_sect_t *ctfsect, const ctf_sect_t *symsect, const void *src; int rc = Z_OK; - if ((base = ctf_data_alloc (size + hdrsz)) == NULL) + if ((base = ctf_alloc (size + hdrsz)) == NULL) return (ctf_set_open_errno (errp, ECTF_ZALLOC)); memcpy (base, ctfsect->cts_data, hdrsz); @@ -1333,7 +1325,7 @@ ctf_bufopen (const ctf_sect_t *ctfsect, const ctf_sect_t *symsect, if ((rc = uncompress (buf, &dstlen, src, srclen)) != Z_OK) { ctf_dprintf ("zlib inflate err: %s\n", zError (rc)); - ctf_data_free (base, size + hdrsz); + free (base); return (ctf_set_open_errno (errp, ECTF_DECOMPRESS)); } @@ -1341,14 +1333,14 @@ ctf_bufopen (const ctf_sect_t *ctfsect, const ctf_sect_t *symsect, { ctf_dprintf ("zlib inflate short -- got %lu of %lu " "bytes\n", (unsigned long) dstlen, (unsigned long) size); - ctf_data_free (base, size + hdrsz); + free (base); return (ctf_set_open_errno (errp, ECTF_CORRUPT)); } } else if (foreign_endian) { - if ((base = ctf_data_alloc (size + hdrsz)) == NULL) + if ((base = ctf_alloc (size + hdrsz)) == NULL) return (ctf_set_open_errno (errp, ECTF_ZALLOC)); } else @@ -1425,14 +1417,6 @@ ctf_bufopen (const ctf_sect_t *ctfsect, const ctf_sect_t *symsect, goto bad; } - /* The ctf region may have been reallocated by init_types(), but now - that is done, it will not move again, so we can protect it, as long - as it didn't come from the ctfsect, which might have been allocated - with malloc(). */ - - if (fp->ctf_base != (void *) ctfsect->cts_data) - ctf_data_protect ((void *) fp->ctf_base, fp->ctf_size); - /* If we have a symbol table section, allocate and initialize the symtab translation table, pointed to by ctf_sxlate. */ @@ -1551,7 +1535,7 @@ ctf_file_close (ctf_file_t *fp) else if (fp->ctf_data_mmapped) ctf_munmap (fp->ctf_data_mmapped, fp->ctf_data_mmapped_len); - ctf_free_base (fp, NULL, 0); + ctf_free_base (fp, NULL); if (fp->ctf_sxlate != NULL) ctf_free (fp->ctf_sxlate); diff --git a/libctf/ctf-subr.c b/libctf/ctf-subr.c index b897351..454716a 100644 --- a/libctf/ctf-subr.c +++ b/libctf/ctf-subr.c @@ -26,49 +26,9 @@ #include #include -static size_t _PAGESIZE _libctf_unused_; int _libctf_version = CTF_VERSION; /* Library client version. */ int _libctf_debug = 0; /* Debugging messages enabled. */ -_libctf_malloc_ void * -ctf_data_alloc (size_t size) -{ - void *ret; - -#ifdef HAVE_MMAP - if (_PAGESIZE == 0) - _PAGESIZE = sysconf(_SC_PAGESIZE); - - if (size > _PAGESIZE) - { - ret = mmap (NULL, size, PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANON, -1, 0); - if (ret == MAP_FAILED) - ret = NULL; - } - else - ret = calloc (1, size); -#else - ret = calloc (1, size); -#endif - return ret; -} - -void -ctf_data_free (void *buf, size_t size _libctf_unused_) -{ -#ifdef HAVE_MMAP - /* Must be the same as the check in ctf_data_alloc(). */ - - if (size > _PAGESIZE) - (void) munmap (buf, size); - else - free (buf); -#else - free (buf); -#endif -} - /* Private, read-only mmap from a file, with fallback to copying. No handling of page-offset issues at all: the caller must allow for that. */ @@ -105,17 +65,6 @@ ctf_munmap (void *buf, size_t length _libctf_unused_) #endif } -void -ctf_data_protect (void *buf _libctf_unused_, size_t size _libctf_unused_) -{ -#ifdef HAVE_MMAP - /* Must be the same as the check in ctf_data_alloc(). */ - - if (size > _PAGESIZE) - (void) mprotect (buf, size, PROT_READ); -#endif -} - _libctf_malloc_ void * ctf_alloc (size_t size) {