pe_ILF_object_p and bfd_check_format_matches
Checks
Commit Message
If pe_ILF_object_p succeeds, pe_ILF_build_a_bfd will have changed the
bfd from being file backed to in-memory. This can have unfortunate
results for targets checked by bfd_check_format_matches after that
point as they will be matching against the created in-memory image
rather than the file. bfd_preserve_restore also has a problem if it
flips the BFD_IN_MEMORY flag, because the flag affects iostream
meaning and should be set if using _bfd_memory_iovec. To fix these
problems, save and restore iostream and iovec along with flags, and
modify bfd_reinit to make the bfd file backed again. Restoring the
iovec and iostream allows the hack in bfd_reinit keeping BFD_IN_MEMORY
(part of BFD_FLAGS_SAVED) to be removed.
One more detail: If restoring from file backed to in-memory then the
bfd needs to be forcibly removed from the cache lru list, since after
the bfd becomes in-memory a bfd_close will delete the bfd's memory
leaving the lru list pointing into freed memory.
* cache.c (bfd_cache_init): Clear BFD_CLOSED_BY_CACHE here..
(bfd_cache_lookup_worker): ..rather than here.
(bfd_cache_close): Comment.
* format.c (struct bfd_preserve): Add iovec and iostream fields.
(bfd_preserve_save): Save them..
(bfd_preserve_restore): ..and restore them, calling
bfd_cache_close if the iovec differs.
(bfd_reinit): Add preserve param. If the bfd has been flipped
to in-memory, reopen the file. Restore flags.
* peicode.h (pe_ILF_cleanup): New function.
(pe_ILF_object_p): Return it.
* bfd.c (BFD_FLAGS_SAVED): Delete.
* bfd-in2.h: Regenerate.
Comments
The last patch wasn't quite correct. bfd_preserve_restore also needs
to handle an in-memory to file backed transition, seen in a testcase
ILF object matching both pei-arm-little and pei-arm-wince-little.
There the first match is saved in preserve_match, and restored at the
end of the bfd_check_format_matches loop making the bfd in-memory. On
finding more than one match the function wants to restore the bfd back
to its original state with another bfd_preserve_restore call before
exiting with a bfd_error_file_ambiguously_recognized error.
It is also not correct to restore abfd->iostream unless the iovec
changes. abfd->iostream is a FILE* when using cache_iovec, and if
the file has been closed and reopened the iostream may have changed.
* format.c (io_reinit): New function.
(bfd_reinit, bfd_preserve_restore): Use it.
diff --git a/bfd/format.c b/bfd/format.c
index dd50b5e653a..66b45ae1979 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -144,6 +144,33 @@ bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve,
sizeof (struct section_hash_entry));
}
+/* A back-end object_p function may flip a bfd from file backed to
+ in-memory, eg. pe_ILF_object_p. In that case to restore the
+ original IO state we need to reopen the file. Conversely, if we
+ are restoring a previously matched pe ILF format and have been
+ checking further target matches using file IO then we need to close
+ the file and detach the bfd from the cache lru list. */
+
+static void
+io_reinit (bfd *abfd, struct bfd_preserve *preserve)
+{
+ if (abfd->iovec != preserve->iovec)
+ {
+ /* Handle file backed to in-memory transition. bfd_cache_close
+ won't do anything unless abfd->iovec is the cache_iovec. */
+ bfd_cache_close (abfd);
+ abfd->iovec = preserve->iovec;
+ abfd->iostream = preserve->iostream;
+ /* Handle in-memory to file backed transition. */
+ if ((abfd->flags & BFD_CLOSED_BY_CACHE) != 0
+ && (abfd->flags & BFD_IN_MEMORY) != 0
+ && (preserve->flags & BFD_CLOSED_BY_CACHE) == 0
+ && (preserve->flags & BFD_IN_MEMORY) == 0)
+ bfd_open_file (abfd);
+ }
+ abfd->flags = preserve->flags;
+}
+
/* Clear out a subset of BFD state. */
static void
@@ -155,16 +182,7 @@ bfd_reinit (bfd *abfd, unsigned int section_id,
cleanup (abfd);
abfd->tdata.any = NULL;
abfd->arch_info = &bfd_default_arch_struct;
- if ((abfd->flags & BFD_CLOSED_BY_CACHE) != 0
- && (abfd->flags & BFD_IN_MEMORY) != 0
- && (preserve->flags & BFD_CLOSED_BY_CACHE) == 0
- && (preserve->flags & BFD_IN_MEMORY) == 0)
- {
- /* This is to reverse pe_ILF_build_a_bfd, which closes the file
- and sets up a bfd in memory. */
- bfd_open_file (abfd);
- }
- abfd->flags = preserve->flags;
+ io_reinit (abfd, preserve);
abfd->build_id = NULL;
bfd_section_list_clear (abfd);
}
@@ -178,11 +196,7 @@ bfd_preserve_restore (bfd *abfd, struct bfd_preserve *preserve)
abfd->tdata.any = preserve->tdata;
abfd->arch_info = preserve->arch_info;
- if (abfd->iovec != preserve->iovec)
- bfd_cache_close (abfd);
- abfd->flags = preserve->flags;
- abfd->iovec = preserve->iovec;
- abfd->iostream = preserve->iostream;
+ io_reinit (abfd, preserve);
abfd->section_htab = preserve->section_htab;
abfd->sections = preserve->sections;
abfd->section_last = preserve->section_last;
On 4/13/23 04:58, Alan Modra via Binutils wrote:
> |The last patch wasn't quite correct.|
Heh, I've just noticed that this change caused a crash when ar is called for ~150 files:
==27734== Invalid read of size 4
==27734== at 0x4D576DD: ftello (in /usr/lib64/libc.so.6)
==27734== by 0x4958E18: close_one (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734== by 0x495F2F4: bfd_open_file (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734== by 0x4B86557: bfd_plugin_open_input (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734== by 0x4B8674F: try_load_plugin (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734== by 0x4B86A5B: bfd_plugin_object_p.lto_priv.0 (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734== by 0x49626EA: bfd_check_format_matches (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734== by 0x4963016: _bfd_write_archive_contents (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734== by 0x4968DD5: bfd_close (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
==27734== by 0x10E9BB: write_archive (in /usr/bin/ar)
==27734== by 0x1103DD: main (in /usr/bin/ar)
==27734== Address 0x0 is not stack'd, malloc'd or (recently) free'd
Martin
On Thu, Apr 13, 2023 at 11:55:10AM +0200, Martin Liška wrote:
> On 4/13/23 04:58, Alan Modra via Binutils wrote:
> > |The last patch wasn't quite correct.|
>
> Heh, I've just noticed that this change caused a crash when ar is called for ~150 files:
>
> ==27734== Invalid read of size 4
> ==27734== at 0x4D576DD: ftello (in /usr/lib64/libc.so.6)
> ==27734== by 0x4958E18: close_one (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
Yes, that will be this part:
> > It is also not correct to restore abfd->iostream unless the iovec
> > changes. abfd->iostream is a FILE* when using cache_iovec, and if
> > the file has been closed and reopened the iostream may have changed.
I saw a similar invalid read in fseeko, and a double free in fclose
due to the above.
On 4/13/23 12:58, Alan Modra wrote:
> On Thu, Apr 13, 2023 at 11:55:10AM +0200, Martin Liška wrote:
>> On 4/13/23 04:58, Alan Modra via Binutils wrote:
>>> |The last patch wasn't quite correct.|
>>
>> Heh, I've just noticed that this change caused a crash when ar is called for ~150 files:
>>
>> ==27734== Invalid read of size 4
>> ==27734== at 0x4D576DD: ftello (in /usr/lib64/libc.so.6)
>> ==27734== by 0x4958E18: close_one (in /usr/lib64/libbfd-2.40.50.20230412-797.so)
>
> Yes, that will be this part:
>>> It is also not correct to restore abfd->iostream unless the iovec
>>> changes. abfd->iostream is a FILE* when using cache_iovec, and if
>>> the file has been closed and reopened the iostream may have changed.
>
> I saw a similar invalid read in fseeko, and a double free in fclose
> due to the above.
>
Good, I can confirm the problem is gone with your latest change.
Thanks,
Martin
@@ -6622,12 +6622,6 @@ struct bfd
/* Compress sections in this BFD with SHF_COMPRESSED zstd. */
#define BFD_COMPRESS_ZSTD 0x400000
- /* Flags bits to be saved in bfd_preserve_save. */
-#define BFD_FLAGS_SAVED \
- (BFD_IN_MEMORY | BFD_COMPRESS | BFD_DECOMPRESS | BFD_LINKER_CREATED \
- | BFD_PLUGIN | BFD_COMPRESS_GABI | BFD_CONVERT_ELF_COMMON \
- | BFD_USE_ELF_STT_COMMON | BFD_COMPRESS_ZSTD)
-
/* Flags bits which are for BFD use only. */
#define BFD_FLAGS_FOR_BFD_USE_MASK \
(BFD_IN_MEMORY | BFD_COMPRESS | BFD_DECOMPRESS | BFD_LINKER_CREATED \
@@ -181,12 +181,6 @@ CODE_FRAGMENT
. {* Compress sections in this BFD with SHF_COMPRESSED zstd. *}
.#define BFD_COMPRESS_ZSTD 0x400000
.
-. {* Flags bits to be saved in bfd_preserve_save. *}
-.#define BFD_FLAGS_SAVED \
-. (BFD_IN_MEMORY | BFD_COMPRESS | BFD_DECOMPRESS | BFD_LINKER_CREATED \
-. | BFD_PLUGIN | BFD_COMPRESS_GABI | BFD_CONVERT_ELF_COMMON \
-. | BFD_USE_ELF_STT_COMMON | BFD_COMPRESS_ZSTD)
-.
. {* Flags bits which are for BFD use only. *}
.#define BFD_FLAGS_FOR_BFD_USE_MASK \
. (BFD_IN_MEMORY | BFD_COMPRESS | BFD_DECOMPRESS | BFD_LINKER_CREATED \
@@ -266,10 +266,7 @@ bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag)
&& !(flag & CACHE_NO_SEEK_ERROR))
bfd_set_error (bfd_error_system_call);
else
- {
- abfd->flags &= ~BFD_CLOSED_BY_CACHE;
- return (FILE *) abfd->iostream;
- }
+ return (FILE *) abfd->iostream;
/* xgettext:c-format */
_bfd_error_handler (_("reopening %pB: %s"),
@@ -506,6 +503,7 @@ bfd_cache_init (bfd *abfd)
}
abfd->iovec = &cache_iovec;
insert (abfd);
+ abfd->flags &= ~BFD_CLOSED_BY_CACHE;
++open_files;
return true;
}
@@ -528,6 +526,7 @@ DESCRIPTION
bool
bfd_cache_close (bfd *abfd)
{
+ /* Don't remove this test. bfd_reinit depends on it. */
if (abfd->iovec != &cache_iovec)
return true;
@@ -99,6 +99,8 @@ struct bfd_preserve
void *marker;
void *tdata;
flagword flags;
+ const struct bfd_iovec *iovec;
+ void *iostream;
const struct bfd_arch_info *arch_info;
struct bfd_section *sections;
struct bfd_section *section_last;
@@ -125,6 +127,8 @@ bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve,
preserve->tdata = abfd->tdata.any;
preserve->arch_info = abfd->arch_info;
preserve->flags = abfd->flags;
+ preserve->iovec = abfd->iovec;
+ preserve->iostream = abfd->iostream;
preserve->sections = abfd->sections;
preserve->section_last = abfd->section_last;
preserve->section_count = abfd->section_count;
@@ -143,14 +147,24 @@ bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve,
/* Clear out a subset of BFD state. */
static void
-bfd_reinit (bfd *abfd, unsigned int section_id, bfd_cleanup cleanup)
+bfd_reinit (bfd *abfd, unsigned int section_id,
+ struct bfd_preserve *preserve, bfd_cleanup cleanup)
{
_bfd_section_id = section_id;
if (cleanup)
cleanup (abfd);
abfd->tdata.any = NULL;
abfd->arch_info = &bfd_default_arch_struct;
- abfd->flags &= BFD_FLAGS_SAVED;
+ if ((abfd->flags & BFD_CLOSED_BY_CACHE) != 0
+ && (abfd->flags & BFD_IN_MEMORY) != 0
+ && (preserve->flags & BFD_CLOSED_BY_CACHE) == 0
+ && (preserve->flags & BFD_IN_MEMORY) == 0)
+ {
+ /* This is to reverse pe_ILF_build_a_bfd, which closes the file
+ and sets up a bfd in memory. */
+ bfd_open_file (abfd);
+ }
+ abfd->flags = preserve->flags;
abfd->build_id = NULL;
bfd_section_list_clear (abfd);
}
@@ -164,7 +178,11 @@ bfd_preserve_restore (bfd *abfd, struct bfd_preserve *preserve)
abfd->tdata.any = preserve->tdata;
abfd->arch_info = preserve->arch_info;
+ if (abfd->iovec != preserve->iovec)
+ bfd_cache_close (abfd);
abfd->flags = preserve->flags;
+ abfd->iovec = preserve->iovec;
+ abfd->iostream = preserve->iostream;
abfd->section_htab = preserve->section_htab;
abfd->sections = preserve->sections;
abfd->section_last = preserve->section_last;
@@ -368,7 +386,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
/* If we already tried a match, the bfd is modified and may
have sections attached, which will confuse the next
_bfd_check_format call. */
- bfd_reinit (abfd, initial_section_id, cleanup);
+ bfd_reinit (abfd, initial_section_id, &preserve, cleanup);
/* Free bfd_alloc memory too. If we have matched and preserved
a target then the high water mark is that much higher. */
if (preserve_match.marker)
@@ -527,7 +545,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
RIGHT_TARG again. */
if (match_targ != right_targ)
{
- bfd_reinit (abfd, initial_section_id, cleanup);
+ bfd_reinit (abfd, initial_section_id, &preserve, cleanup);
bfd_release (abfd, preserve.marker);
if (bfd_seek (abfd, (file_ptr) 0, SEEK_SET) != 0)
goto err_ret;
@@ -1158,6 +1158,17 @@ pe_ILF_build_a_bfd (bfd * abfd,
return false;
}
+/* Cleanup function, returned from check_format hook. */
+
+static void
+pe_ILF_cleanup (bfd *abfd)
+{
+ struct bfd_in_memory *bim = abfd->iostream;
+ free (bim->buffer);
+ free (bim);
+ abfd->iostream = NULL;
+}
+
/* We have detected an Import Library Format archive element.
Decode the element and return the appropriate target. */
@@ -1331,7 +1342,7 @@ pe_ILF_object_p (bfd * abfd)
return NULL;
}
- return _bfd_no_cleanup;
+ return pe_ILF_cleanup;
}
static void