Re: PATCH: binutils/2467: "ar q" / ranlib has large memory use (linear in archive size)

["H. J. Lu" <hjl at lucon dot org>]

On Thu, Mar 23, 2006 at 09:17:35PM -0800, H. J. Lu wrote:
> On Fri, Mar 24, 2006 at 11:41:39AM +1030, Alan Modra wrote:
> > On Mon, Mar 20, 2006 at 06:23:03PM -0800, H. J. Lu wrote:
> > > Will this patch break anything?
> > 
> > I don't know the answer to that, but really, it's something that you as
> > patch submitter should answer.  You could gain a reasonable level of
> 
> I think it is safe. _bfd_generic_bfd_free_cached_info is called from
> _bfd_compute_and_write_armap only. It has no problems with gcc build.
> 
> > confidence by allocating and scribbling on some memory after you've
> > freed the objalloc.  Test it with a coff an aout target too.
> > 
> 
> I added
> 
>   abfd->sections = NULL;
>   abfd->section_last = NULL;
>   abfd->outsymbols = NULL;
>   abfd->tdata.any = NULL;
>   abfd->usrdata = NULL;
>   abfd->memory = NULL;
> 
> It doesn't cause any trouble. I checked i386-coff and i386-aout.
> 

It breaks "strip -g" on archive. The problem is

1. bfd_close on archive member calls _bfd_write_archive_contents.
2. It calls _bfd_compute_and_write_armap.
3. It calls bfd_free_cached_info on archive member.
4. Then bfd_close calls _bfd_delete_bfd on archive member, which tries
to free the same memory.

I rewrote the patch to limit it to ELF and check memory before free
it.


H.J.
----
bfd/

2006-04-06  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/2467
	* elf.c (_bfd_elf_close_and_cleanup): Check elf_tdata (abfd)
	is NULL first.

	* elf32-arm.c (elf32_arm_close_and_cleanup): Check if
	abfd->sections is NULL.
	(elf32_arm_bfd_free_cached_info): New.
	(bfd_elf32_bfd_free_cached_info): Defined.

	* elfxx-target.h (bfd_elfNN_bfd_free_cached_info): Default it
	to _bfd_free_cached_info.

	* libbfd-in.h (_bfd_free_cached_info): New.
	* libbfd: Regenerated.

	* opncls.c (_bfd_delete_bfd): Check if abfd->memory is NULL.
	(_bfd_free_cached_info): New.

binutils/testsuite/

2006-04-06  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/2467
	* binutils-all/objcopy.exp (strip_test): Also test "strip -g"
	on archive.

--- binutils/bfd/elf.c.cached	2006-03-16 12:37:42.000000000 -0800
+++ binutils/bfd/elf.c	2006-04-06 16:25:59.000000000 -0700
@@ -7171,7 +7171,7 @@ _bfd_elf_close_and_cleanup (bfd *abfd)
 {
   if (bfd_get_format (abfd) == bfd_object)
     {
-      if (elf_shstrtab (abfd) != NULL)
+      if (elf_tdata (abfd) != NULL && elf_shstrtab (abfd) != NULL)
 	_bfd_elf_strtab_free (elf_shstrtab (abfd));
       _bfd_dwarf2_cleanup_debug_info (abfd);
     }
--- binutils/bfd/elf32-arm.c.cached	2006-03-16 12:37:42.000000000 -0800
+++ binutils/bfd/elf32-arm.c	2006-04-06 16:27:29.000000000 -0700
@@ -7936,11 +7936,25 @@ unrecord_section_via_map_over_sections (
 static bfd_boolean
 elf32_arm_close_and_cleanup (bfd * abfd)
 {
-  bfd_map_over_sections (abfd, unrecord_section_via_map_over_sections, NULL);
+  if (abfd->sections)
+    bfd_map_over_sections (abfd,
+			   unrecord_section_via_map_over_sections,
+			   NULL);
 
   return _bfd_elf_close_and_cleanup (abfd);
 }
 
+static bfd_boolean
+elf32_arm_bfd_free_cached_info (bfd * abfd)
+{
+  if (abfd->sections)
+    bfd_map_over_sections (abfd,
+			   unrecord_section_via_map_over_sections,
+			   NULL);
+
+  return _bfd_free_cached_info (abfd);
+}
+
 /* Display STT_ARM_TFUNC symbols as functions.  */
 
 static void
@@ -8100,6 +8114,7 @@ const struct elf_size_info elf32_arm_siz
 #define bfd_elf32_new_section_hook		elf32_arm_new_section_hook
 #define bfd_elf32_bfd_is_target_special_symbol	elf32_arm_is_target_special_symbol
 #define bfd_elf32_close_and_cleanup             elf32_arm_close_and_cleanup
+#define bfd_elf32_bfd_free_cached_info          elf32_arm_bfd_free_cached_info
 #define bfd_elf32_bfd_final_link		elf32_arm_bfd_final_link
 
 #define elf_backend_get_symbol_type             elf32_arm_get_symbol_type
--- binutils/bfd/elfxx-target.h.cached	2006-02-27 15:50:54.000000000 -0800
+++ binutils/bfd/elfxx-target.h	2006-04-06 16:16:35.000000000 -0700
@@ -29,7 +29,9 @@
 #ifndef bfd_elfNN_close_and_cleanup
 #define	bfd_elfNN_close_and_cleanup _bfd_elf_close_and_cleanup
 #endif
-#define bfd_elfNN_bfd_free_cached_info _bfd_generic_bfd_free_cached_info
+#ifndef bfd_elfNN_bfd_free_cached_info
+#define bfd_elfNN_bfd_free_cached_info _bfd_free_cached_info
+#endif
 #ifndef bfd_elfNN_get_section_contents
 #define bfd_elfNN_get_section_contents _bfd_generic_get_section_contents
 #endif
--- binutils/bfd/libbfd-in.h.cached	2006-03-16 12:37:43.000000000 -0800
+++ binutils/bfd/libbfd-in.h	2006-04-06 16:14:01.000000000 -0700
@@ -155,6 +155,8 @@ bfd * _bfd_new_bfd
   (void);
 void _bfd_delete_bfd
   (bfd *);
+bfd_boolean _bfd_free_cached_info
+  (bfd *);
 
 bfd_boolean bfd_false
   (bfd *ignore);
--- binutils/bfd/opncls.c.cached	2006-03-16 12:37:43.000000000 -0800
+++ binutils/bfd/opncls.c	2006-04-06 16:12:31.000000000 -0700
@@ -115,11 +115,35 @@ _bfd_new_bfd_contained_in (bfd *obfd)
 void
 _bfd_delete_bfd (bfd *abfd)
 {
-  bfd_hash_table_free (&abfd->section_htab);
-  objalloc_free ((struct objalloc *) abfd->memory);
+  if (abfd->memory)
+    {
+      bfd_hash_table_free (&abfd->section_htab);
+      objalloc_free ((struct objalloc *) abfd->memory);
+    }
   free (abfd);
 }
 
+/* Free objalloc memory.  */
+
+bfd_boolean
+_bfd_free_cached_info (bfd *abfd)
+{
+  if (abfd->memory)
+    {
+      bfd_hash_table_free (&abfd->section_htab);
+      objalloc_free ((struct objalloc *) abfd->memory);
+
+      abfd->sections = NULL;
+      abfd->section_last = NULL;
+      abfd->outsymbols = NULL;
+      abfd->tdata.any = NULL;
+      abfd->usrdata = NULL;
+      abfd->memory = NULL;
+    }
+
+  return TRUE;
+}
+
 /*
 SECTION
 	Opening and closing BFDs
--- binutils/binutils/testsuite/binutils-all/objcopy.exp.cached	2005-10-20 10:06:40.000000000 -0700
+++ binutils/binutils/testsuite/binutils-all/objcopy.exp	2006-04-04 07:32:21.000000000 -0700
@@ -359,6 +359,12 @@ proc strip_test { } {
 	return
     }
 
+    set exec_output [binutils_run $STRIP "-g $archive"]
+    if ![string match "" $exec_output] {
+	fail $test
+	return
+    }
+
     set exec_output [binutils_run $STRIP "$STRIPFLAGS $archive"]
     if ![string match "" $exec_output] {
 	fail $test