This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Simple Objloader patch


Hi Anthony

Anthony Tonizzo wrote:

> Davy Wouters pointed out a memory leak on the objoader
> package last month. The attached patch fixes the problem
> and adds a few more checks on returned values.

Thank you for the patch.

I discovered a couple of CDL issues during testing and I've also cleaned
out the Cygwin tclsh workarounds which are now obsolete and have been
removed elsewhere. Everything is now checked-in. Final patch attached.

John Dallaway

Index: services/objloader/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/services/objloader/current/ChangeLog,v
retrieving revision 1.6
diff -U5 -r1.6 ChangeLog
--- services/objloader/current/ChangeLog	3 Jul 2009 14:49:44 -0000	1.6
+++ services/objloader/current/ChangeLog	9 Oct 2009 13:21:48 -0000
@@ -1,5 +1,18 @@
+2009-10-09  John Dallaway  <john@dallaway.org.uk>
+
+	* cdl/objloader.cdl: Eliminate workarounds for file path handling
+	issue in obsolete Cygwin tclsh. Use ACTUAL_CFLAGS for robustness.
+
+2009-09-13  Anthony Tonizzo  <atonizzo@gmail.com>
+
+	* src/objloader.c, src/objelf.c, include/objelf.h : Fixed a memory
+	leak where a library section was loaded but memory allocated was not
+	released. This bug was reported by Davy Wouters on the eCos list.
+	Added minor cosmetics and a number of CYG_ASSERT and if() to test the
+	values returnedby cyg_ldr_load_elf_section().
+
 2009-07-03  John Dallaway  <john@dallaway.org.uk>
 
 	* cdl/objloader.cdl, src/objelf.c, src/relocate_ppc.c,
 	  src/relocate_arm.c: Eliminate dependency on CYGPKG_IO_FILEIO when
 	the filesystem loader is not required.
Index: services/objloader/current/include/objelf.h
===================================================================
RCS file: /cvs/ecos/ecos/packages/services/objloader/current/include/objelf.h,v
retrieving revision 1.5
diff -U5 -r1.5 objelf.h
--- services/objloader/current/include/objelf.h	3 Jul 2009 14:49:44 -0000	1.5
+++ services/objloader/current/include/objelf.h	9 Oct 2009 13:21:49 -0000
@@ -7,11 +7,11 @@
  *
  * ================================================================= 
  * ####ECOSGPLCOPYRIGHTBEGIN####                                     
  * -------------------------------------------                       
  * This file is part of eCos, the Embedded Configurable Operating System.
- * Copyright (C) 2005, 2008 Free Software Foundation, Inc.                 
+ * Copyright (C) 2005, 2008, 2009 Free Software Foundation, Inc.                 
  *
  * eCos is free software; you can redistribute it and/or modify it under
  * the terms of the GNU General Public License as published by the Free
  * Software Foundation; either version 2 or (at your option) any later
  * version.                                                          
@@ -96,13 +96,13 @@
 
 //==============================================================================
 // Debug functions.
 
 #if CYGPKG_SERVICES_OBJLOADER_DEBUG_LEVEL > 0
-void       cyg_ldr_print_section_data(PELF_OBJECT);
-void       cyg_ldr_print_symbol_names(PELF_OBJECT);
-void       cyg_ldr_print_rel_names(PELF_OBJECT);
+void      cyg_ldr_print_section_data(PELF_OBJECT);
+void      cyg_ldr_print_symbol_names(PELF_OBJECT);
+cyg_int32 cyg_ldr_print_rel_names(PELF_OBJECT);
 #endif
 
 //==============================================================================
 // Internal functions
 
Index: services/objloader/current/src/objelf.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/services/objloader/current/src/objelf.c,v
retrieving revision 1.5
diff -U5 -r1.5 objelf.c
--- services/objloader/current/src/objelf.c	3 Jul 2009 14:49:45 -0000	1.5
+++ services/objloader/current/src/objelf.c	9 Oct 2009 13:21:49 -0000
@@ -49,10 +49,11 @@
  * 
  * =================================================================
  */
 
 #include <cyg/infra/diag.h>     // For diagnostic printing.
+#include <cyg/infra/cyg_ass.h>
 #include <cyg/hal/hal_tables.h>
 #include <stdio.h>
 
 #include <pkgconf/objloader.h>
 #include <cyg/objloader/elf.h>
@@ -114,11 +115,11 @@
                     p_symtab[i].st_shndx,
                     p_strtab + p_symtab[i].st_name);
     diag_printf("\n");
 }
 
-void 
+cyg_int32
 cyg_ldr_print_rel_names(PELF_OBJECT p)
 {
     int        i, j, r_entries, sym_index;
     Elf32_Sym *p_symtab = (Elf32_Sym*)p->sections[p->hdrndx_symtab];
     char      *p_strtab = (char*)p->sections[p->hdrndx_strtab];
@@ -133,20 +134,24 @@
     for (i = 1; i < p->p_elfhdr->e_shnum; i++)
     {
         if ((p->p_sechdr[i].sh_type == SHT_REL) ||
                                   (p->p_sechdr[i].sh_type == SHT_RELA))
         {                                  
-            // Calculate the total number of entries in the .rela section.
+            // Calculate the total number of entries in the .rela/.rel section.
             r_entries = p->p_sechdr[i].sh_size / p->p_sechdr[i].sh_entsize;
 
             diag_printf("\n\nSymbols at: %s\n\n", 
                          p_shstrtab + p->p_sechdr[i].sh_name);
 #if ELF_ARCH_RELTYPE == Elf_Rela        
-            p_rela = (Elf32_Rela*)cyg_ldr_load_elf_section(p, i);
+            p_rela = (Elf32_Rela *)cyg_ldr_load_elf_section(p, i);
+            if (p_rela == 0)
+                return -1;
             printf("Offset    Info      Name [+ Addend]\n");
 #else
-            p_rel = (Elf32_Rel*)cyg_ldr_load_elf_section(p, i);
+            p_rel = (Elf32_Rel *)cyg_ldr_load_elf_section(p, i);
+            if (p_rel == 0)
+                return -1;
             printf("Offset    Info     Name\n");
 #endif
 
             for (j = 0; j < r_entries; j++)
             {
@@ -286,21 +291,25 @@
 cyg_int32 
 cyg_ldr_relocate_section(PELF_OBJECT p, cyg_uint32 r_shndx)
 {
     int         i, rc;
 #if ELF_ARCH_RELTYPE == Elf_Rela        
-    Elf32_Rela* p_rela = (Elf32_Rela*)cyg_ldr_load_elf_section(p, r_shndx);
+    Elf32_Rela *p_rela = (Elf32_Rela *)cyg_ldr_load_elf_section(p, r_shndx);
+    if (p_rela == 0)
+        return -1;
 #else
-    Elf32_Rel*  p_rel = (Elf32_Rel*)cyg_ldr_load_elf_section(p, r_shndx);
+    Elf32_Rel *p_rel = (Elf32_Rel *)cyg_ldr_load_elf_section(p, r_shndx);
+    if (p_rel == 0)
+        return -1;
 #endif
 
 #if CYGPKG_SERVICES_OBJLOADER_DEBUG_LEVEL > 0
-    Elf32_Sym *p_symtab = (Elf32_Sym*)cyg_ldr_section_address(p, 
+    Elf32_Sym *p_symtab = (Elf32_Sym *)cyg_ldr_section_address(p, 
                                                            p->hdrndx_symtab);
-    char      *p_strtab = (char*)cyg_ldr_section_address(p, p->hdrndx_strtab);
-    char      *p_shstrtab = (char*)cyg_ldr_section_address(p, 
-                                                     p->p_elfhdr->e_shstrndx);
+    char *p_strtab = (char *)cyg_ldr_section_address(p, p->hdrndx_strtab);
+    char *p_shstrtab = (char *)cyg_ldr_section_address(p, 
+                                                       p->p_elfhdr->e_shstrndx);
 #endif
 
     // Now we can get the address of the contents of the section to modify.
     cyg_uint32 r_target_shndx = p->p_sechdr[r_shndx].sh_info;
     cyg_uint32 r_target_addr  = (cyg_uint32)cyg_ldr_section_address(p, 
@@ -309,11 +318,11 @@
 #if CYGPKG_SERVICES_OBJLOADER_DEBUG_LEVEL > 0
     diag_printf("Relocating section \"%s\"\n",
             p_shstrtab + p->p_sechdr[r_target_shndx].sh_name);
     diag_printf("----------------------------------------\n"); 
 #if CYGPKG_SERVICES_OBJLOADER_DEBUG_LEVEL > 1
-    diag_printf(" Ndx  Type             Offset   Name\"\n");
+    diag_printf(" Ndx  Type             Offset    Name\n");
 #endif
 #endif
 
     // Perform relocatation for each of the members of this table.
     cyg_uint32 r_entries = p->p_sechdr[r_shndx].sh_size / 
Index: services/objloader/current/src/objloader.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/services/objloader/current/src/objloader.c,v
retrieving revision 1.5
diff -U5 -r1.5 objloader.c
--- services/objloader/current/src/objloader.c	3 Jul 2009 14:49:45 -0000	1.5
+++ services/objloader/current/src/objloader.c	9 Oct 2009 13:21:50 -0000
@@ -6,11 +6,11 @@
  *
  * ================================================================= 
  * ####ECOSGPLCOPYRIGHTBEGIN####                                     
  * -------------------------------------------                       
  * This file is part of eCos, the Embedded Configurable Operating System.
- * Copyright (C) 2005, 2008 Free Software Foundation, Inc.                 
+ * Copyright (C) 2005, 2008, 2009 Free Software Foundation, Inc.                 
  *
  * eCos is free software; you can redistribute it and/or modify it under
  * the terms of the GNU General Public License as published by the Free
  * Software Foundation; either version 2 or (at your option) any later
  * version.                                                          
@@ -79,10 +79,12 @@
 }
 
 void
 cyg_ldr_delete_elf_section(PELF_OBJECT p, cyg_uint32 idx)
 {
+    if (p->sections[idx] == 0)
+        return;
     cyg_ldr_free(p->sections[idx]);
     p->sections[idx] = 0; 
 }    
 
 // Frees all the memory allocated for a particular ELF object. Also calls
@@ -92,11 +94,11 @@
 cyg_ldr_free_elf_object(PELF_OBJECT p)
 {
     cyg_int32 i;
         
     for (i = 0; i < p->p_elfhdr->e_shnum + 1; i++)
-        if (p->sections[i])
+        if (p->sections[i] != 0)
             cyg_ldr_delete_elf_section(p, i);
 
     if (p->sections != 0)
         cyg_ldr_free(p->sections); 
 
@@ -140,20 +142,31 @@
 // Allocates memory and loads the contents of a specific ELF section.
 // Returns the address of the newly allocated memory, of 0 for any error.
 cyg_uint32 
 *cyg_ldr_load_elf_section(PELF_OBJECT p, cyg_uint32 idx)
 {
-    cyg_uint32 *addr = (cyg_uint32 *)cyg_ldr_malloc(p->p_sechdr[idx].sh_size);
-    CYG_ASSERT(addr != 0, "Cannot malloc() section");
-    if (addr == 0)
+    // Make sure we are not requesting the loading of a section for which we
+    //  have no pointer.
+    CYG_ASSERT(idx < p->p_elfhdr->e_shnum + 1, "Invalid section id.");
+    
+    // If this section has already been loaded its pointer is already available
+    //  in the sections[] array.
+    if (p->sections[idx] != 0)
+        return p->sections[idx];
+    p->sections[idx] = (cyg_uint32)cyg_ldr_malloc(p->p_sechdr[idx].sh_size);
+    CYG_ASSERT(p->sections[idx] != 0, "Cannot malloc() section");
+    if (p->sections[idx] == 0)
     {
         cyg_ldr_last_error = "ERROR IN MALLOC";
         return (void*)0;
     }
     p->seek(p, p->p_sechdr[idx].sh_offset);
-    p->read(p, sizeof(char), p->p_sechdr[idx].sh_size, addr);
-    return addr;
+    p->read(p,
+            sizeof(char),
+            p->p_sechdr[idx].sh_size,
+            (void *)p->sections[idx]);
+    return p->sections[idx];
 }    
 
 // Returns the starting address of a section. If the section is not already
 //  loaded in memory, area for it will be allocated and the section will be
 //  loaded.
@@ -275,11 +288,13 @@
     p->read(p, p->p_elfhdr->e_shentsize, p->p_elfhdr->e_shnum, p->p_sechdr);
     
     // Load the section header string table. This is a byte oriented table,
     //  so alignment is not an issue.
     idx = p->p_elfhdr->e_shstrndx;
-    p->sections[idx] = cyg_ldr_load_elf_section(p, idx);
+    cyg_uint32 section_addr = cyg_ldr_load_elf_section(p, idx);
+    if (section_addr == 0)
+        return -1;
     return 0;
 }
 
 PELF_OBJECT
 cyg_ldr_open_library(CYG_ADDRWORD ptr, cyg_int32 mode)
@@ -337,11 +352,11 @@
         // Now look for the index of .symtab. These are the symbols needed for 
         //  the symbol retrieval as well as relocation.
         if (!strcmp(p_shstrtab + e_obj->p_sechdr[i].sh_name, ELF_STRING_symtab))
         {              
             e_obj->hdrndx_symtab = i;
-            e_obj->sections[i] = cyg_ldr_load_elf_section(e_obj, i);
+            cyg_ldr_load_elf_section(e_obj, i);
             if (e_obj->sections[i] == 0)
             {
                 cyg_ldr_free_elf_object(e_obj);
                 return 0;
             }    
@@ -351,11 +366,11 @@
         //  to compare the name of external references symbols against the
         //  names in the in the CYG_HAL_TABLE provided by the user.
         if (!strcmp(p_shstrtab + e_obj->p_sechdr[i].sh_name, ELF_STRING_strtab))
         {              
             e_obj->hdrndx_strtab = i;
-            e_obj->sections[i] = cyg_ldr_load_elf_section(e_obj, i);
+            cyg_ldr_load_elf_section(e_obj, i);
             if (e_obj->sections[i] == 0)
             {
                 cyg_ldr_free_elf_object(e_obj);
                 return 0;
             }    
Index: services/objloader/current/cdl/objloader.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/services/objloader/current/cdl/objloader.cdl,v
retrieving revision 1.6
diff -U5 -r1.6 objloader.cdl
--- services/objloader/current/cdl/objloader.cdl	3 Jul 2009 14:49:44 -0000	1.6
+++ services/objloader/current/cdl/objloader.cdl	9 Oct 2009 13:21:49 -0000
@@ -189,29 +189,27 @@
         description   "
             This option enables the building of a library and an
             application for testing the loader." 
         
         make -priority 320 {
-            tests/testromfs/hello.o : <PACKAGE>/tests/library/hello.c
-            @sh -c "mkdir -p tests tests/testromfs"
-            $(CC) -c $(INCLUDE_PATH) -I$(dir $<) $(CFLAGS) -o $@ $<
+            testobj/hello.o : <PACKAGE>/tests/library/hello.c
+            @mkdir -p "$(dir $@)"
+            $(CC) -c $(INCLUDE_PATH) -I$(dir $<) $(ACTUAL_CFLAGS) -o $@ $<
         }
 
         make -priority 322 {
-            <PREFIX>/include/cyg/objloader/testromfs_be.h : tests/testromfs/hello.o 
-            $(PREFIX)/bin/mk_romfs -b tests/testromfs testromfs_be.bin 
-	    @mkdir -p "$(dir $@)"
-            @tclsh $(PREFIX)/bin/file2c.tcl testromfs_be.bin testromfs_be.h
-            @cp testromfs_be.h $@
+            <PREFIX>/include/cyg/objloader/testromfs_be.h : testobj/hello.o
+            $(PREFIX)/bin/mk_romfs -b $(dir $<) testromfs_be.bin
+            @mkdir -p "$(dir $@)"
+            @tclsh $(PREFIX)/bin/file2c.tcl testromfs_be.bin $@
         }
         
         make -priority 322 {
-            <PREFIX>/include/cyg/objloader/testromfs_le.h : tests/testromfs/hello.o 
-            $(PREFIX)/bin/mk_romfs tests/testromfs testromfs_le.bin
-	    @mkdir -p "$(dir $@)"
-            @tclsh $(PREFIX)/bin/file2c.tcl testromfs_le.bin testromfs_le.h
-            @cp testromfs_le.h $@
+            <PREFIX>/include/cyg/objloader/testromfs_le.h : testobj/hello.o
+            $(PREFIX)/bin/mk_romfs $(dir $<) testromfs_le.bin
+            @mkdir -p "$(dir $@)"
+            @tclsh $(PREFIX)/bin/file2c.tcl testromfs_le.bin $@
         }
     
         cdl_option CYGPKG_OBJLOADER_TESTS {
             display     "Objloader tests"
             flavor      data
Index: fs/rom/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/fs/rom/current/ChangeLog,v
retrieving revision 1.27
diff -U5 -r1.27 ChangeLog
--- fs/rom/current/ChangeLog	28 Apr 2009 13:06:11 -0000	1.27
+++ fs/rom/current/ChangeLog	9 Oct 2009 13:21:50 -0000
@@ -1,8 +1,14 @@
+2009-10-09  John Dallaway  <john@dallaway.org.uk>
+
+	* cdl/romfs.cdl: Eliminate workarounds for file path handling
+	issue in obsolete Cygwin tclsh. Build the mk_romfs tool with higher
+	priority than the custom rules which use it.
+
 2009-04-28  John Dallaway  <john@dallaway.org.uk>
 
-	cdl/romfs.cdl: Use CYGPKG_IO_FILEIO as the parent.
+	* cdl/romfs.cdl: Use CYGPKG_IO_FILEIO as the parent.
 
 2009-02-07  John Dallaway  <john@dallaway.org.uk>
 
 	* cdl/romfs.cdl: Pass file2c.tcl to tclsh directly.
 
Index: fs/rom/current/cdl/romfs.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/fs/rom/current/cdl/romfs.cdl,v
retrieving revision 1.15
diff -U5 -r1.15 romfs.cdl
--- fs/rom/current/cdl/romfs.cdl	28 Apr 2009 13:06:11 -0000	1.15
+++ fs/rom/current/cdl/romfs.cdl	9 Oct 2009 13:21:50 -0000
@@ -67,16 +67,21 @@
     cdl_option CYGBLD_FS_ROMFS_MK_ROMFS {
         display       "Build the tool used to build filesystems"
         flavor        bool
         default_value 1
 
+        make -priority 98 {
+            <PREFIX>/bin/file2c.tcl: <PACKAGE>/support/file2c.tcl
+            @mkdir -p "$(dir $@)"
+            @cp $< $@
+        }
+
         # FIXME: host compiler/flags should be provided by config system
-        make -priority 100 {
-            <PREFIX>/bin/mk_romfs: <PACKAGE>/support/mk_romfs.c <PREFIX>/bin/file2c.tcl
+        make -priority 98 {
+            <PREFIX>/bin/mk_romfs: <PACKAGE>/support/mk_romfs.c
             @mkdir -p "$(dir $@)"
             @$(HOST_CC) -g -O2 -o $@ $< || cc -g -O2 -o $@ $< || gcc -g -O2 -o $@ $<
-            @cp $(REPOSITORY)/$(PACKAGE)/support/file2c.tcl $(PREFIX)/bin
         }
 
         description "
             When enabled this option will build a host tool which can be
             used to create a rom filesystem image."
@@ -110,28 +115,21 @@
         requires      CYGBLD_FS_ROMFS_MK_ROMFS        
         description   "
                 This option enables the building of the ROM filesystem tests."
 
         make -priority 100 {
-            <PREFIX>/include/cyg/romfs/testromfs_le.h : <PREFIX>/bin/mk_romfs <PACKAGE>/support/file2c.tcl
-            $(PREFIX)/bin/mk_romfs $(REPOSITORY)/$(PACKAGE)/tests/testromfs testromfs_le.bin
+            <PREFIX>/include/cyg/romfs/testromfs_le.h : <PACKAGE>/tests/testromfs <PREFIX>/bin/mk_romfs <PREFIX>/bin/file2c.tcl
+            $(PREFIX)/bin/mk_romfs $< testromfs_le.bin
             @mkdir -p "$(dir $@)"
-            # work around cygwin path problems by copying to build dir
-            @cp $(REPOSITORY)/$(PACKAGE)/support/file2c.tcl .
-            tclsh file2c.tcl testromfs_le.bin testromfs_le.h
-            @rm -f $@ file2c.tcl
-            @cp testromfs_le.h $@
+            tclsh $(PREFIX)/bin/file2c.tcl testromfs_le.bin $@
         }
     
         make -priority 100 {
-            <PREFIX>/include/cyg/romfs/testromfs_be.h : <PREFIX>/bin/mk_romfs <PACKAGE>/support/file2c.tcl
-            $(PREFIX)/bin/mk_romfs -b $(REPOSITORY)/$(PACKAGE)/tests/testromfs testromfs_be.bin
+            <PREFIX>/include/cyg/romfs/testromfs_be.h : <PACKAGE>/tests/testromfs <PREFIX>/bin/mk_romfs <PREFIX>/bin/file2c.tcl
+            $(PREFIX)/bin/mk_romfs -b $< testromfs_be.bin
             @mkdir -p "$(dir $@)"
-            # work around cygwin path problems by copying to bin dir
-            @cp $(REPOSITORY)/$(PACKAGE)/support/file2c.tcl $(PREFIX)/bin/
-            tclsh $(PREFIX)/bin/file2c.tcl testromfs_be.bin testromfs_be.h
-            @cp testromfs_be.h $@
+            tclsh $(PREFIX)/bin/file2c.tcl testromfs_be.bin $@
         }
     
         cdl_option CYGPKG_FS_ROM_TESTS {
             display "ROM filesystem tests"
             flavor  data

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]