This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 3/5] remove deleted BFDs from the archive cache
Alan> Tom said he'd look into fixing the leak this causes, so I'm happy
Alan> to leave that to him. :)
Here's the patch.
I think it would be good for someone to double check it.
I wrote this patch by searching for all the places that could allocate
an areltdata and changing them to use bfd_zmalloc.
Then I examined all the users of bfd->arelt_data and all callers of
bfd_read_ar_hdr (and _bfd_read_ar_hdr) to see how the data was used.
This revealed a number of spots that used bfd_release to free this data.
Finally, I added a free to _bfd_delete_bfd.
I once again built ld and all the binutils programs with -lmcheck, and
then ran the test suites. These all passed. I also ran a single 'ar'
test (one that was failing yesterday) plus the new 'bfdtest1' test under
valgrind. These were also clean.
I found a few oddities in BFD while working on this patch:
* _bfd_get_elt_at_filepos can release new_areldata but still leave a
stale pointer in n_nfd->arelt_data. I fixed this. I am not sure if
this can ever result in a bug, but I think paranoia is preferable.
* bfd_slurp_bsd_armap_f2 leaks 'mapdata' before the patch -- it frees it
on the error path but not on the normal path. I fixed this.
* _bfd_xcoff_read_ar_hdr currently allocates 'ret' with bfd_alloc. I
think it should clear it instead; I did this.
Also, this function already assumes 'ret' is malloced, which is a
latent bug.
One final note on arelt_data: right now it is a void* in the BFD. It
seems to me that it would be just as opaque, and more type-safe, to
change the structure name to 'struct bfd_areltdata', then use this name
in the BFD -- but leave the struct type incomplete so that library
clients can't dereference it.
Tom
2012-08-17 Tom Tromey <tromey@redhat.com>
* vms-lib.c (_bfd_vms_lib_get_module): Use bfd_zmalloc for
areltdata.
* opncls.c (_bfd_delete_bfd): Free arelt_data.
* mach-o.c (bfd_mach_o_fat_member_init): Use bfd_zmalloc for
areltdata.
* ecoff.c (_bfd_ecoff_slurp_armap): Use free for mapdata.
* coff-rs6000.c (_bfd_xcoff_read_ar_hdr): Use bfd_zmalloc for
areltdata.
(xcoff_write_archive_contents_old): Likewise.
(xcoff_write_archive_contents_big): Likewise.
* archive64.c (bfd_elf64_archive_slurp_armap): Use free for
areltdata.
* archive.c (_bfd_generic_read_ar_hdr_mag): Use bfd_zmalloc and
free for areltdata.
(_bfd_get_elt_at_filepos): Likewise. Clear n_nfd->arelt_data on
failure.
(do_slurp_bsd_armap): Use bfd_zmalloc and free for areltdata.
(do_slurp_coff_armap): Likewise.
(_bfd_slurp_extended_name_table): Likewise.
(bfd_slurp_bsd_armap_f2): Likewise. Don't leak 'mapdata'.
diff --git a/bfd/archive.c b/bfd/archive.c
index 2d67e1f..4abee07 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -517,7 +517,7 @@ _bfd_generic_read_ar_hdr_mag (bfd *abfd, const char *mag)
parsed_size -= namelen;
extra_size = namelen;
- allocptr = (char *) bfd_zalloc (abfd, allocsize);
+ allocptr = (char *) bfd_zmalloc (allocsize);
if (allocptr == NULL)
return NULL;
filename = (allocptr
@@ -560,7 +560,7 @@ _bfd_generic_read_ar_hdr_mag (bfd *abfd, const char *mag)
if (!allocptr)
{
- allocptr = (char *) bfd_zalloc (abfd, allocsize);
+ allocptr = (char *) bfd_zmalloc (allocsize);
if (allocptr == NULL)
return NULL;
}
@@ -655,13 +655,13 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
if (ext_arch == NULL
|| ! bfd_check_format (ext_arch, bfd_archive))
{
- bfd_release (archive, new_areldata);
+ free (new_areldata);
return NULL;
}
n_nfd = _bfd_get_elt_at_filepos (ext_arch, new_areldata->origin);
if (n_nfd == NULL)
{
- bfd_release (archive, new_areldata);
+ free (new_areldata);
return NULL;
}
n_nfd->proxy_origin = bfd_tell (archive);
@@ -683,7 +683,7 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
if (n_nfd == NULL)
{
- bfd_release (archive, new_areldata);
+ free (new_areldata);
return NULL;
}
@@ -707,7 +707,8 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
if (_bfd_add_bfd_to_archive_cache (archive, filepos, n_nfd))
return n_nfd;
- bfd_release (archive, new_areldata);
+ free (new_areldata);
+ n_nfd->arelt_data = NULL;
return NULL;
}
@@ -894,7 +895,7 @@ do_slurp_bsd_armap (bfd *abfd)
if (mapdata == NULL)
return FALSE;
parsed_size = mapdata->parsed_size;
- bfd_release (abfd, mapdata); /* Don't need it any more. */
+ free (mapdata);
raw_armap = (bfd_byte *) bfd_zalloc (abfd, parsed_size);
if (raw_armap == NULL)
@@ -970,7 +971,7 @@ do_slurp_coff_armap (bfd *abfd)
if (mapdata == NULL)
return FALSE;
parsed_size = mapdata->parsed_size;
- bfd_release (abfd, mapdata); /* Don't need it any more. */
+ free (mapdata);
if (bfd_bread (int_buf, 4, abfd) != 4)
{
@@ -1063,7 +1064,7 @@ do_slurp_coff_armap (bfd *abfd)
ardata->first_file_filepos +=
(tmp->parsed_size + sizeof (struct ar_hdr) + 1) & ~(unsigned) 1;
}
- bfd_release (abfd, tmp);
+ free (tmp);
}
}
@@ -1180,15 +1181,17 @@ bfd_slurp_bsd_armap_f2 (bfd *abfd)
if (mapdata->parsed_size < HPUX_SYMDEF_COUNT_SIZE + BSD_STRING_COUNT_SIZE)
{
+ free (mapdata);
wrong_format:
bfd_set_error (bfd_error_wrong_format);
byebye:
- bfd_release (abfd, mapdata);
return FALSE;
}
left = mapdata->parsed_size - HPUX_SYMDEF_COUNT_SIZE - BSD_STRING_COUNT_SIZE;
amt = mapdata->parsed_size;
+ free (mapdata);
+
raw_armap = (bfd_byte *) bfd_zalloc (abfd, amt);
if (raw_armap == NULL)
goto byebye;
@@ -1290,7 +1293,7 @@ _bfd_slurp_extended_name_table (bfd *abfd)
if (bfd_ardata (abfd)->extended_names == NULL)
{
byebye:
- bfd_release (abfd, namedata);
+ free (namedata);
return FALSE;
}
@@ -1327,8 +1330,7 @@ _bfd_slurp_extended_name_table (bfd *abfd)
bfd_ardata (abfd)->first_file_filepos +=
(bfd_ardata (abfd)->first_file_filepos) % 2;
- /* FIXME, we can't release namedata here because it was allocated
- below extended_names on the objalloc... */
+ free (namedata);
}
return TRUE;
}
diff --git a/bfd/archive64.c b/bfd/archive64.c
index f3a13d3..db4ce2c 100644
--- a/bfd/archive64.c
+++ b/bfd/archive64.c
@@ -77,7 +77,7 @@ bfd_elf64_archive_slurp_armap (bfd *abfd)
if (mapdata == NULL)
return FALSE;
parsed_size = mapdata->parsed_size;
- bfd_release (abfd, mapdata);
+ free (mapdata);
if (bfd_bread (int_buf, 8, abfd) != 8)
{
diff --git a/bfd/coff-rs6000.c b/bfd/coff-rs6000.c
index 9326b32..edbef95 100644
--- a/bfd/coff-rs6000.c
+++ b/bfd/coff-rs6000.c
@@ -1496,7 +1496,7 @@ _bfd_xcoff_read_ar_hdr (bfd *abfd)
struct areltdata *ret;
bfd_size_type amt = sizeof (struct areltdata);
- ret = (struct areltdata *) bfd_alloc (abfd, amt);
+ ret = (struct areltdata *) bfd_zmalloc (amt);
if (ret == NULL)
return NULL;
@@ -2113,7 +2113,7 @@ xcoff_write_archive_contents_old (bfd *abfd)
total_namlen += strlen (normalize_filename (sub)) + 1;
if (sub->arelt_data == NULL)
{
- sub->arelt_data = bfd_zalloc (sub, sizeof (struct areltdata));
+ sub->arelt_data = bfd_zmalloc (sizeof (struct areltdata));
if (sub->arelt_data == NULL)
return FALSE;
}
@@ -2329,7 +2329,7 @@ xcoff_write_archive_contents_big (bfd *abfd)
if (current_bfd->arelt_data == NULL)
{
size = sizeof (struct areltdata);
- current_bfd->arelt_data = bfd_zalloc (current_bfd, size);
+ current_bfd->arelt_data = bfd_zmalloc (size);
if (current_bfd->arelt_data == NULL)
return FALSE;
}
diff --git a/bfd/ecoff.c b/bfd/ecoff.c
index 3b65c0e..eaf8ada 100644
--- a/bfd/ecoff.c
+++ b/bfd/ecoff.c
@@ -2904,7 +2904,7 @@ _bfd_ecoff_slurp_armap (bfd *abfd)
if (mapdata == NULL)
return FALSE;
parsed_size = mapdata->parsed_size;
- bfd_release (abfd, (void *) mapdata);
+ free (mapdata);
raw_armap = (char *) bfd_alloc (abfd, parsed_size);
if (raw_armap == NULL)
diff --git a/bfd/mach-o.c b/bfd/mach-o.c
index 84d5a72..0379f4f 100644
--- a/bfd/mach-o.c
+++ b/bfd/mach-o.c
@@ -4294,7 +4294,7 @@ bfd_mach_o_fat_member_init (bfd *abfd,
abfd->filename = name;
}
- areltdata = bfd_zalloc (abfd, sizeof (struct areltdata));
+ areltdata = bfd_zmalloc (sizeof (struct areltdata));
areltdata->parsed_size = entry->size;
abfd->arelt_data = areltdata;
abfd->iostream = NULL;
diff --git a/bfd/opncls.c b/bfd/opncls.c
index b2ed9be..fdccba3 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -136,6 +136,7 @@ _bfd_delete_bfd (bfd *abfd)
objalloc_free ((struct objalloc *) abfd->memory);
}
+ free (abfd->arelt_data);
free (abfd);
}
diff --git a/bfd/vms-lib.c b/bfd/vms-lib.c
index fa23b78..56b80ad 100644
--- a/bfd/vms-lib.c
+++ b/bfd/vms-lib.c
@@ -1,6 +1,6 @@
/* BFD back-end for VMS archive files.
- Copyright 2010, 2011 Free Software Foundation, Inc.
+ Copyright 2010, 2011, 2012 Free Software Foundation, Inc.
Written by Tristan Gingold <gingold@adacore.com>, AdaCore.
This file is part of BFD, the Binary File Descriptor library.
@@ -1337,7 +1337,7 @@ _bfd_vms_lib_get_module (bfd *abfd, unsigned int modidx)
res = _bfd_create_empty_archive_element_shell (abfd);
if (res == NULL)
return NULL;
- arelt = bfd_zalloc (res, sizeof (*arelt));
+ arelt = bfd_zmalloc (sizeof (*arelt));
if (arelt == NULL)
return NULL;
res->arelt_data = arelt;