From 5c4ce239a3ab9263163a0fe8e7487b39a41c43d1 Mon Sep 17 00:00:00 2001 From: Alan Modra Date: Tue, 5 Jun 2018 22:31:23 +0930 Subject: [PATCH] Tidy bfdio to consistenly use containing archive Archive element IO is performed on the file of the containing archive, which leads to the BFD "where" field of archives and their elements being out of sync with the real file position. (We're talking traditional archives here, not thin archives.) The old bfd_seek code recognized this by not attempting to optimize away seeks for archives. However, there was other code that could return bogus results. For example, cache.c limits the number of open files by closing a file and remembering its state once the limit is reached. If bfd_tell is called on an archive element when the containing archive is closed, it will return an invalid file pointer. It's possible to have a valid "where" field for archives by always using and updating the containing archive BFD. That's what this patch does. Note that cache.c used to find the containing archive BFD anyway for the iostream, so we're not really doing extra work, just transferring it up to the correct abstraction level. The patch also gets rid of some hacks. bfd_tell was called when bfd_seek failed, in an attempt to correct "where". That's got to be papering over another problem, so that code has been removed. bfd_read also had an "optimiziation" to return early when the number of bytes was zero, and bfd_seek optimized calls that didn't move the file pointer. This was covering for a coff_slurp_line_table bug where IO was attempted on a pe-dll BFD without an iovec. * bfd.c (struct bfd): Update comment on "where" usage. * bfdio.c (bfd_bwrite, bfd_stat): Use and update "iovec", "iostream", and "where" from containing archive file. Return error on NULL iovec. (bfd_bread): Similarly, and return error attempted out of bounds archive element access. (bfd_tell, bfd_flush): Use and update "iovec", "iostream", and "where" from containing archive file. (bfd_seek): Likewise. Return error on NULL iovec. Don't attempt to optimize away seeks. Don't paper over errors by calling bfd_tell. (bfd_get_mtime): Call bfd_stat rather than iovec->bstat. (bfd_get_size): Likewise. (bfd_mmap): Operate on and use iovec of containing archive file. Return error on NULL iovec. * cache.c (bfd_cache_lookup_worker): Abort if working on archive element bfd. (cache_bread_1): Delete bfd parameter, add FILE* parameter. Don't ignore zero byte reads. (cache_bread): Look up FILE* in cache here. Error on NULL lookup. (cache_bwrite): Rename "where" to "from". (cache_bmmap): Don't handle archive elements. * coffcode.h (coff_slurp_line_table): Exit early on zero lineno count. * bfd-in2.h: Regenerate. --- bfd/ChangeLog | 29 +++++++ bfd/bfd-in2.h | 5 +- bfd/bfd.c | 5 +- bfd/bfdio.c | 202 ++++++++++++++++++++++++------------------------- bfd/cache.c | 43 ++++------- bfd/coffcode.h | 3 + 6 files changed, 151 insertions(+), 136 deletions(-) diff --git a/bfd/ChangeLog b/bfd/ChangeLog index 3688cf29aa6..8b144916c25 100644 --- a/bfd/ChangeLog +++ b/bfd/ChangeLog @@ -1,3 +1,32 @@ +2018-06-05 Alan Modra + + * bfd.c (struct bfd): Update comment on "where" usage. + * bfdio.c (bfd_bwrite, bfd_stat): Use and update "iovec", + "iostream", and "where" from containing archive file. Return + error on NULL iovec. + (bfd_bread): Similarly, and return error attempted out of + bounds archive element access. + (bfd_tell, bfd_flush): Use and update "iovec", "iostream", and + "where" from containing archive file. + (bfd_seek): Likewise. Return error on NULL iovec. Don't + attempt to optimize away seeks. Don't paper over errors by + calling bfd_tell. + (bfd_get_mtime): Call bfd_stat rather than iovec->bstat. + (bfd_get_size): Likewise. + (bfd_mmap): Operate on and use iovec of containing archive + file. Return error on NULL iovec. + * cache.c (bfd_cache_lookup_worker): Abort if working on + archive element bfd. + (cache_bread_1): Delete bfd parameter, add FILE* parameter. + Don't ignore zero byte reads. + (cache_bread): Look up FILE* in cache here. Error on NULL + lookup. + (cache_bwrite): Rename "where" to "from". + (cache_bmmap): Don't handle archive elements. + * coffcode.h (coff_slurp_line_table): Exit early on zero + lineno count. + * bfd-in2.h: Regenerate. + 2018-06-05 Alan Modra PR 23254 diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h index 65735f026e6..5ceb935d270 100644 --- a/bfd/bfd-in2.h +++ b/bfd/bfd-in2.h @@ -6789,8 +6789,9 @@ struct bfd least-recently-used list of BFDs. */ struct bfd *lru_prev, *lru_next; - /* When a file is closed by the caching routines, BFD retains - state information on the file here... */ + /* Track current file position (or current buffer offset for + in-memory BFDs). When a file is closed by the caching routines, + BFD retains state information on the file here. */ ufile_ptr where; /* File modified time, if mtime_set is TRUE. */ diff --git a/bfd/bfd.c b/bfd/bfd.c index 09c817df4f5..851710401e0 100644 --- a/bfd/bfd.c +++ b/bfd/bfd.c @@ -74,8 +74,9 @@ CODE_FRAGMENT . least-recently-used list of BFDs. *} . struct bfd *lru_prev, *lru_next; . -. {* When a file is closed by the caching routines, BFD retains -. state information on the file here... *} +. {* Track current file position (or current buffer offset for +. in-memory BFDs). When a file is closed by the caching routines, +. BFD retains state information on the file here. *} . ufile_ptr where; . . {* File modified time, if mtime_set is TRUE. *} diff --git a/bfd/bfdio.c b/bfd/bfdio.c index 37d30d42787..136fa8b1d3f 100644 --- a/bfd/bfdio.c +++ b/bfd/bfdio.c @@ -176,27 +176,40 @@ DESCRIPTION bfd_size_type bfd_bread (void *ptr, bfd_size_type size, bfd *abfd) { - size_t nread; + file_ptr nread; + bfd *element_bfd = abfd; + ufile_ptr offset = 0; + + while (abfd->my_archive != NULL + && !bfd_is_thin_archive (abfd->my_archive)) + { + offset += abfd->origin; + abfd = abfd->my_archive; + } /* If this is an archive element, don't read past the end of this element. */ - if (abfd->arelt_data != NULL) + if (element_bfd->arelt_data != NULL) { - bfd_size_type maxbytes = arelt_size (abfd); + bfd_size_type maxbytes = arelt_size (element_bfd); - if (abfd->where + size > maxbytes) + if (abfd->where < offset || abfd->where - offset >= maxbytes) { - if (abfd->where >= maxbytes) - return 0; - size = maxbytes - abfd->where; + bfd_set_error (bfd_error_invalid_operation); + return -1; } + if (abfd->where - offset + size > maxbytes) + size = maxbytes - (abfd->where - offset); } - if (abfd->iovec) - nread = abfd->iovec->bread (abfd, ptr, size); - else - nread = 0; - if (nread != (size_t) -1) + if (abfd->iovec == NULL) + { + bfd_set_error (bfd_error_invalid_operation); + return -1; + } + + nread = abfd->iovec->bread (abfd, ptr, size); + if (nread != -1) abfd->where += nread; return nread; @@ -205,16 +218,22 @@ bfd_bread (void *ptr, bfd_size_type size, bfd *abfd) bfd_size_type bfd_bwrite (const void *ptr, bfd_size_type size, bfd *abfd) { - size_t nwrote; + file_ptr nwrote; - if (abfd->iovec) - nwrote = abfd->iovec->bwrite (abfd, ptr, size); - else - nwrote = 0; + while (abfd->my_archive != NULL + && !bfd_is_thin_archive (abfd->my_archive)) + abfd = abfd->my_archive; + + if (abfd->iovec == NULL) + { + bfd_set_error (bfd_error_invalid_operation); + return -1; + } - if (nwrote != (size_t) -1) + nwrote = abfd->iovec->bwrite (abfd, ptr, size); + if (nwrote != -1) abfd->where += nwrote; - if (nwrote != size) + if ((bfd_size_type) nwrote != size) { #ifdef ENOSPC errno = ENOSPC; @@ -227,33 +246,35 @@ bfd_bwrite (const void *ptr, bfd_size_type size, bfd *abfd) file_ptr bfd_tell (bfd *abfd) { + ufile_ptr offset = 0; file_ptr ptr; - if (abfd->iovec) + while (abfd->my_archive != NULL + && !bfd_is_thin_archive (abfd->my_archive)) { - bfd *parent_bfd = abfd; - ptr = abfd->iovec->btell (abfd); - - while (parent_bfd->my_archive != NULL - && !bfd_is_thin_archive (parent_bfd->my_archive)) - { - ptr -= parent_bfd->origin; - parent_bfd = parent_bfd->my_archive; - } + offset += abfd->origin; + abfd = abfd->my_archive; } - else - ptr = 0; + if (abfd->iovec == NULL) + return 0; + + ptr = abfd->iovec->btell (abfd); abfd->where = ptr; - return ptr; + return ptr - offset; } int bfd_flush (bfd *abfd) { - if (abfd->iovec) - return abfd->iovec->bflush (abfd); - return 0; + while (abfd->my_archive != NULL + && !bfd_is_thin_archive (abfd->my_archive)) + abfd = abfd->my_archive; + + if (abfd->iovec == NULL) + return 0; + + return abfd->iovec->bflush (abfd); } /* Returns 0 for success, negative value for failure (in which case @@ -263,11 +284,17 @@ bfd_stat (bfd *abfd, struct stat *statbuf) { int result; - if (abfd->iovec) - result = abfd->iovec->bstat (abfd, statbuf); - else - result = -1; + while (abfd->my_archive != NULL + && !bfd_is_thin_archive (abfd->my_archive)) + abfd = abfd->my_archive; + if (abfd->iovec == NULL) + { + bfd_set_error (bfd_error_invalid_operation); + return -1; + } + + result = abfd->iovec->bstat (abfd, statbuf); if (result < 0) bfd_set_error (bfd_error_system_call); return result; @@ -280,79 +307,48 @@ int bfd_seek (bfd *abfd, file_ptr position, int direction) { int result; - file_ptr file_position; - /* For the time being, a BFD may not seek to it's end. The problem - is that we don't easily have a way to recognize the end of an - element in an archive. */ - - BFD_ASSERT (direction == SEEK_SET || direction == SEEK_CUR); + ufile_ptr offset = 0; - if (direction == SEEK_CUR && position == 0) - return 0; - - if (abfd->my_archive == NULL || bfd_is_thin_archive (abfd->my_archive)) - { - if (direction == SEEK_SET && (bfd_vma) position == abfd->where) - return 0; - } - else + while (abfd->my_archive != NULL + && !bfd_is_thin_archive (abfd->my_archive)) { - /* We need something smarter to optimize access to archives. - Currently, anything inside an archive is read via the file - handle for the archive. Which means that a bfd_seek on one - component affects the `current position' in the archive, as - well as in any other component. - - It might be sufficient to put a spike through the cache - abstraction, and look to the archive for the file position, - but I think we should try for something cleaner. - - In the meantime, no optimization for archives. */ + offset += abfd->origin; + abfd = abfd->my_archive; } - file_position = position; - if (direction == SEEK_SET) + if (abfd->iovec == NULL) { - bfd *parent_bfd = abfd; - - while (parent_bfd->my_archive != NULL - && !bfd_is_thin_archive (parent_bfd->my_archive)) - { - file_position += parent_bfd->origin; - parent_bfd = parent_bfd->my_archive; - } + bfd_set_error (bfd_error_invalid_operation); + return -1; } - if (abfd->iovec) - result = abfd->iovec->bseek (abfd, file_position, direction); - else - result = -1; + /* For the time being, a BFD may not seek to it's end. The problem + is that we don't easily have a way to recognize the end of an + element in an archive. */ + BFD_ASSERT (direction == SEEK_SET || direction == SEEK_CUR); + + if (direction != SEEK_CUR) + position += offset; + result = abfd->iovec->bseek (abfd, position, direction); if (result != 0) { - int hold_errno = errno; - - /* Force redetermination of `where' field. */ - bfd_tell (abfd); - /* An EINVAL error probably means that the file offset was absurd. */ - if (hold_errno == EINVAL) + if (errno == EINVAL) bfd_set_error (bfd_error_file_truncated); else - { - bfd_set_error (bfd_error_system_call); - errno = hold_errno; - } + bfd_set_error (bfd_error_system_call); } else { /* Adjust `where' field. */ - if (direction == SEEK_SET) - abfd->where = position; - else + if (direction == SEEK_CUR) abfd->where += position; + else + abfd->where = position; } + return result; } @@ -377,10 +373,7 @@ bfd_get_mtime (bfd *abfd) if (abfd->mtime_set) return abfd->mtime; - if (abfd->iovec == NULL) - return 0; - - if (abfd->iovec->bstat (abfd, &buf) != 0) + if (bfd_stat (abfd, &buf) != 0) return 0; abfd->mtime = buf.st_mtime; /* Save value in case anyone wants it */ @@ -425,10 +418,7 @@ bfd_get_size (bfd *abfd) { struct stat buf; - if (abfd->iovec == NULL) - return 0; - - if (abfd->iovec->bstat (abfd, &buf) != 0) + if (bfd_stat (abfd, &buf) != 0) return 0; return buf.st_size; @@ -479,10 +469,18 @@ bfd_mmap (bfd *abfd, void *addr, bfd_size_type len, int prot, int flags, file_ptr offset, void **map_addr, bfd_size_type *map_len) { - void *ret = (void *)-1; + while (abfd->my_archive != NULL + && !bfd_is_thin_archive (abfd->my_archive)) + { + offset += abfd->origin; + abfd = abfd->my_archive; + } if (abfd->iovec == NULL) - return ret; + { + bfd_set_error (bfd_error_invalid_operation); + return (void *) -1; + } return abfd->iovec->bmmap (abfd, addr, len, prot, flags, offset, map_addr, map_len); diff --git a/bfd/cache.c b/bfd/cache.c index 4b14043ec82..faee6779f8f 100644 --- a/bfd/cache.c +++ b/bfd/cache.c @@ -237,13 +237,12 @@ close_one (void) static FILE * bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag) { - bfd *orig_bfd = abfd; if ((abfd->flags & BFD_IN_MEMORY) != 0) abort (); - while (abfd->my_archive != NULL - && !bfd_is_thin_archive (abfd->my_archive)) - abfd = abfd->my_archive; + if (abfd->my_archive != NULL + && !bfd_is_thin_archive (abfd->my_archive)) + abort (); if (abfd->iostream != NULL) { @@ -271,7 +270,7 @@ bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag) /* xgettext:c-format */ _bfd_error_handler (_("reopening %pB: %s\n"), - orig_bfd, bfd_errmsg (bfd_get_error ())); + abfd, bfd_errmsg (bfd_get_error ())); return NULL; } @@ -301,25 +300,9 @@ cache_bseek (struct bfd *abfd, file_ptr offset, int whence) first octet in the file, NOT the beginning of the archive header. */ static file_ptr -cache_bread_1 (struct bfd *abfd, void *buf, file_ptr nbytes) +cache_bread_1 (FILE *f, void *buf, file_ptr nbytes) { - FILE *f; file_ptr nread; - /* FIXME - this looks like an optimization, but it's really to cover - up for a feature of some OSs (not solaris - sigh) that - ld/pe-dll.c takes advantage of (apparently) when it creates BFDs - internally and tries to link against them. BFD seems to be smart - enough to realize there are no symbol records in the "file" that - doesn't exist but attempts to read them anyway. On Solaris, - attempting to read zero bytes from a NULL file results in a core - dump, but on other platforms it just returns zero bytes read. - This makes it to something reasonable. - DJ */ - if (nbytes == 0) - return 0; - - f = bfd_cache_lookup (abfd, CACHE_NORMAL); - if (f == NULL) - return 0; #if defined (__VAX) && defined (VMS) /* Apparently fread on Vax VMS does not keep the record length @@ -355,6 +338,11 @@ static file_ptr cache_bread (struct bfd *abfd, void *buf, file_ptr nbytes) { file_ptr nread = 0; + FILE *f; + + f = bfd_cache_lookup (abfd, CACHE_NORMAL); + if (f == NULL) + return -1; /* Some filesystems are unable to handle reads that are too large (for instance, NetApp shares with oplocks turned off). To avoid @@ -368,7 +356,7 @@ cache_bread (struct bfd *abfd, void *buf, file_ptr nbytes) if (chunk_size > max_chunk_size) chunk_size = max_chunk_size; - chunk_nread = cache_bread_1 (abfd, (char *) buf + nread, chunk_size); + chunk_nread = cache_bread_1 (f, (char *) buf + nread, chunk_size); /* Update the nread count. @@ -389,14 +377,14 @@ cache_bread (struct bfd *abfd, void *buf, file_ptr nbytes) } static file_ptr -cache_bwrite (struct bfd *abfd, const void *where, file_ptr nbytes) +cache_bwrite (struct bfd *abfd, const void *from, file_ptr nbytes) { file_ptr nwrite; FILE *f = bfd_cache_lookup (abfd, CACHE_NORMAL); if (f == NULL) return 0; - nwrite = fwrite (where, 1, nbytes, f); + nwrite = fwrite (from, 1, nbytes, f); if (nwrite < nbytes && ferror (f)) { bfd_set_error (bfd_error_system_call); @@ -468,11 +456,6 @@ cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED, if (pagesize_m1 == 0) pagesize_m1 = getpagesize () - 1; - /* Handle archive members. */ - if (abfd->my_archive != NULL - && !bfd_is_thin_archive (abfd->my_archive)) - offset += abfd->origin; - /* Align. */ pg_offset = offset & ~pagesize_m1; pg_len = (len + (offset - pg_offset) + pagesize_m1) & ~pagesize_m1; diff --git a/bfd/coffcode.h b/bfd/coffcode.h index 2ca32059cb4..11c22ab67e6 100644 --- a/bfd/coffcode.h +++ b/bfd/coffcode.h @@ -4265,6 +4265,9 @@ coff_slurp_line_table (bfd *abfd, asection *asect) bfd_boolean have_func; bfd_boolean ret = TRUE; + if (asect->lineno_count == 0) + return TRUE; + BFD_ASSERT (asect->lineno == NULL); if (asect->lineno_count > asect->size) -- 2.30.2