* Patch to detect invalid mergeable string sections @ 2007-10-30 16:19 Joseph S. Myers 2007-10-30 23:39 ` Alan Modra 0 siblings, 1 reply; 7+ messages in thread From: Joseph S. Myers @ 2007-10-30 16:19 UTC (permalink / raw) To: binutils GCC versions before 4.1.2 could generate object files with invalid mergeable string sections containing unterminated strings. This was fixed by: 2006-06-21 Jakub Jelinek <jakub@redhat.com> * varasm.c (mergeable_string_section): Check for embedded NULs and NUL termination in the first int_size_in_bytes (TREE_TYPE (decl)) rather than TREE_STRING_LENGTH bytes. When linking such object files, the linker can give internal errors from _bfd_merged_section_offset (or possibly fail in other ways; looking for the end of such a string will run off the end of the section and start reading uninitialized memory). Invalid input should never cause internal errors from the linker; it should give normal non-internal errors to the user diagnosing the invalid input instead. This patch adds a check that the strings found in such sections in input files do not run off the end of their sections. A failure at this point in turn requires two other checks for NULL secinfo to be inserted so later stages in the attempted merging don't dereference a NULL pointer. Tested on i686-pc-linux-gnu (native). OK to commit? bfd: 2007-10-30 Joseph Myers <joseph@codesourcery.com> * merge.c (sec_merge_hash_lookup): Add parameter sec_end. Check for unterminated strings. All callers changed. (_bfd_write_merged_section, _bfd_merged_section_offset): Handle NULL secinfo from merge failures. ld/testsuite: 2007-10-30 Joseph Myers <joseph@codesourcery.com> * ld-elf/merge3.d, ld-elf/merge3.s: New. Index: bfd/merge.c =================================================================== RCS file: /cvs/src/src/bfd/merge.c,v retrieving revision 1.33 diff -u -r1.33 merge.c --- bfd/merge.c 19 Sep 2007 12:08:34 -0000 1.33 +++ bfd/merge.c 30 Oct 2007 16:00:19 -0000 @@ -133,6 +133,7 @@ static struct sec_merge_hash_entry * sec_merge_hash_lookup (struct sec_merge_hash *table, const char *string, + const unsigned char *sec_end, unsigned int alignment, bfd_boolean create) { register const unsigned char *s; @@ -154,6 +155,12 @@ hash += c + (c << 17); hash ^= hash >> 2; ++len; + if (sec_end && s >= sec_end) + { + (*_bfd_error_handler) + (_("unterminated string in section marked for merging")); + return NULL; + } } hash += len + (len << 17); } @@ -161,6 +168,12 @@ { for (;;) { + if (sec_end && s + table->entsize > sec_end) + { + (*_bfd_error_handler) + (_("unterminated string in section marked for merging")); + return NULL; + } for (i = 0; i < table->entsize; ++i) if (s[i] != '\0') break; @@ -264,7 +277,9 @@ { register struct sec_merge_hash_entry *entry; - entry = sec_merge_hash_lookup (tab, str, alignment, TRUE); + entry = sec_merge_hash_lookup (tab, str, + secinfo->contents + secinfo->sec->size, + alignment, TRUE); if (entry == NULL) return NULL; @@ -779,6 +794,9 @@ secinfo = (struct sec_merge_sec_info *) psecinfo; + if (!secinfo) + return FALSE; + if (secinfo->first_str == NULL) return TRUE; @@ -807,6 +825,9 @@ secinfo = (struct sec_merge_sec_info *) psecinfo; + if (!secinfo) + return 0; + if (offset >= sec->rawsize) { if (offset > sec->rawsize) @@ -849,7 +870,7 @@ { p = secinfo->contents + (offset / sec->entsize) * sec->entsize; } - entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, 0, FALSE); + entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, NULL, 0, FALSE); if (!entry) { if (! secinfo->htab->strings) Index: ld/testsuite/ld-elf/merge3.d =================================================================== RCS file: ld/testsuite/ld-elf/merge3.d diff -N ld/testsuite/ld-elf/merge3.d --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ ld/testsuite/ld-elf/merge3.d 30 Oct 2007 16:00:20 -0000 @@ -0,0 +1,3 @@ +#source: merge3.s +#ld: -T merge.ld +#error: unterminated string in section marked for merging Index: ld/testsuite/ld-elf/merge3.s =================================================================== RCS file: ld/testsuite/ld-elf/merge3.s diff -N ld/testsuite/ld-elf/merge3.s --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ ld/testsuite/ld-elf/merge3.s 30 Oct 2007 16:00:20 -0000 @@ -0,0 +1,7 @@ + .section .rodata.str,"aMS","progbits",1 +.LC0: + .ascii "abcd" + .text + .global _start +_start: + .long .LC0 -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch to detect invalid mergeable string sections 2007-10-30 16:19 Patch to detect invalid mergeable string sections Joseph S. Myers @ 2007-10-30 23:39 ` Alan Modra 2007-10-31 18:17 ` Joseph S. Myers 0 siblings, 1 reply; 7+ messages in thread From: Alan Modra @ 2007-10-30 23:39 UTC (permalink / raw) To: Joseph S. Myers; +Cc: binutils On Tue, Oct 30, 2007 at 04:10:18PM +0000, Joseph S. Myers wrote: > + if (sec_end && s >= sec_end) > + { > + (*_bfd_error_handler) > + (_("unterminated string in section marked for merging")); > + return NULL; > + } It would be nice if this error specified the section. I think you can do that by simply returning NULL here and calling _bfd_error_handler at error_return in record_section after testing for bfd_get_error returning anything but bfd_error_no_memory. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch to detect invalid mergeable string sections 2007-10-30 23:39 ` Alan Modra @ 2007-10-31 18:17 ` Joseph S. Myers 2007-11-01 7:11 ` Alan Modra 0 siblings, 1 reply; 7+ messages in thread From: Joseph S. Myers @ 2007-10-31 18:17 UTC (permalink / raw) To: Alan Modra; +Cc: binutils On Wed, 31 Oct 2007, Alan Modra wrote: > On Tue, Oct 30, 2007 at 04:10:18PM +0000, Joseph S. Myers wrote: > > + if (sec_end && s >= sec_end) > > + { > > + (*_bfd_error_handler) > > + (_("unterminated string in section marked for merging")); > > + return NULL; > > + } > > It would be nice if this error specified the section. I think you can > do that by simply returning NULL here and calling _bfd_error_handler > at error_return in record_section after testing for bfd_get_error > returning anything but bfd_error_no_memory. How about this revised patch which does that? bfd: 2007-10-31 Joseph Myers <joseph@codesourcery.com> * merge.c (sec_merge_hash_lookup): Add parameter sec_end. Check for unterminated strings. All callers changed. (record_section): Add parameter abfd. Give error message for unterminated strings. (_bfd_merge_sections): Update call to record_section. (_bfd_write_merged_section, _bfd_merged_section_offset): Handle NULL secinfo from merge failures. ld/testsuite: 2007-10-31 Joseph Myers <joseph@codesourcery.com> * ld-elf/merge3.d, ld-elf/merge3.s: New. Index: bfd/merge.c =================================================================== RCS file: /cvs/src/src/bfd/merge.c,v retrieving revision 1.33 diff -u -p -r1.33 merge.c --- bfd/merge.c 19 Sep 2007 12:08:34 -0000 1.33 +++ bfd/merge.c 31 Oct 2007 17:58:02 -0000 @@ -133,6 +133,7 @@ sec_merge_hash_newfunc (struct bfd_hash_ static struct sec_merge_hash_entry * sec_merge_hash_lookup (struct sec_merge_hash *table, const char *string, + const unsigned char *sec_end, unsigned int alignment, bfd_boolean create) { register const unsigned char *s; @@ -154,6 +155,8 @@ sec_merge_hash_lookup (struct sec_merge_ hash += c + (c << 17); hash ^= hash >> 2; ++len; + if (sec_end && s >= sec_end) + return NULL; } hash += len + (len << 17); } @@ -161,6 +164,8 @@ sec_merge_hash_lookup (struct sec_merge_ { for (;;) { + if (sec_end && s + table->entsize > sec_end) + return NULL; for (i = 0; i < table->entsize; ++i) if (s[i] != '\0') break; @@ -264,7 +269,9 @@ sec_merge_add (struct sec_merge_hash *ta { register struct sec_merge_hash_entry *entry; - entry = sec_merge_hash_lookup (tab, str, alignment, TRUE); + entry = sec_merge_hash_lookup (tab, str, + secinfo->contents + secinfo->sec->size, + alignment, TRUE); if (entry == NULL) return NULL; @@ -436,7 +443,8 @@ _bfd_add_merge_section (bfd *abfd, void /* Record one section into the hash table. */ static bfd_boolean -record_section (struct sec_merge_info *sinfo, +record_section (bfd *abfd, + struct sec_merge_info *sinfo, struct sec_merge_sec_info *secinfo) { asection *sec = secinfo->sec; @@ -513,6 +521,10 @@ record_section (struct sec_merge_info *s return TRUE; error_return: + if (bfd_get_error () != bfd_error_no_memory) + (*_bfd_error_handler) + (_("%B: unterminated string in section `%A' marked for merging"), + abfd, sec); for (secinfo = sinfo->chain; secinfo; secinfo = secinfo->next) *secinfo->psecinfo = NULL; return FALSE; @@ -695,7 +707,7 @@ alloc_failure: with _bfd_merge_section. */ bfd_boolean -_bfd_merge_sections (bfd *abfd ATTRIBUTE_UNUSED, +_bfd_merge_sections (bfd *abfd, struct bfd_link_info *info ATTRIBUTE_UNUSED, void *xsinfo, void (*remove_hook) (bfd *, asection *)) @@ -722,7 +734,7 @@ _bfd_merge_sections (bfd *abfd ATTRIBUTE if (remove_hook) (*remove_hook) (abfd, secinfo->sec); } - else if (! record_section (sinfo, secinfo)) + else if (! record_section (abfd, sinfo, secinfo)) break; if (secinfo) @@ -779,6 +791,9 @@ _bfd_write_merged_section (bfd *output_b secinfo = (struct sec_merge_sec_info *) psecinfo; + if (!secinfo) + return FALSE; + if (secinfo->first_str == NULL) return TRUE; @@ -807,6 +822,9 @@ _bfd_merged_section_offset (bfd *output_ secinfo = (struct sec_merge_sec_info *) psecinfo; + if (!secinfo) + return 0; + if (offset >= sec->rawsize) { if (offset > sec->rawsize) @@ -849,7 +867,7 @@ _bfd_merged_section_offset (bfd *output_ { p = secinfo->contents + (offset / sec->entsize) * sec->entsize; } - entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, 0, FALSE); + entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, NULL, 0, FALSE); if (!entry) { if (! secinfo->htab->strings) Index: ld/testsuite/ld-elf/merge3.d =================================================================== RCS file: ld/testsuite/ld-elf/merge3.d diff -N ld/testsuite/ld-elf/merge3.d --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ ld/testsuite/ld-elf/merge3.d 31 Oct 2007 17:58:03 -0000 @@ -0,0 +1,3 @@ +#source: merge3.s +#ld: -T merge.ld +#error: unterminated string in section `.rodata.str' marked for merging Index: ld/testsuite/ld-elf/merge3.s =================================================================== RCS file: ld/testsuite/ld-elf/merge3.s diff -N ld/testsuite/ld-elf/merge3.s --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ ld/testsuite/ld-elf/merge3.s 31 Oct 2007 17:58:03 -0000 @@ -0,0 +1,7 @@ + .section .rodata.str,"aMS","progbits",1 +.LC0: + .ascii "abcd" + .text + .global _start +_start: + .long .LC0 -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch to detect invalid mergeable string sections 2007-10-31 18:17 ` Joseph S. Myers @ 2007-11-01 7:11 ` Alan Modra 2007-11-05 2:12 ` Alan Modra 0 siblings, 1 reply; 7+ messages in thread From: Alan Modra @ 2007-11-01 7:11 UTC (permalink / raw) To: Joseph S. Myers; +Cc: binutils On Wed, Oct 31, 2007 at 06:07:56PM +0000, Joseph S. Myers wrote: > bfd: > 2007-10-31 Joseph Myers <joseph@codesourcery.com> > > * merge.c (sec_merge_hash_lookup): Add parameter sec_end. Check > for unterminated strings. All callers changed. > (record_section): Add parameter abfd. Give error message for > unterminated strings. > (_bfd_merge_sections): Update call to record_section. > (_bfd_write_merged_section, _bfd_merged_section_offset): Handle > NULL secinfo from merge failures. > > ld/testsuite: > 2007-10-31 Joseph Myers <joseph@codesourcery.com> > > * ld-elf/merge3.d, ld-elf/merge3.s: New. OK. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch to detect invalid mergeable string sections 2007-11-01 7:11 ` Alan Modra @ 2007-11-05 2:12 ` Alan Modra 2007-11-05 2:31 ` Joseph S. Myers 0 siblings, 1 reply; 7+ messages in thread From: Alan Modra @ 2007-11-05 2:12 UTC (permalink / raw) To: Joseph S. Myers, binutils Joseph, I'm reverting most of your patch and instead providing the missing zero terminator at the end of a string merge section. (My SLES 10 box system compiler happens to be a 4.1.2 prerelease without Jakub's fix and I hit your error message on the ld bootstrap tests..) Since versions of gcc with the bug are widely used, I think ld should fix the problem, especially since the fix is easy. From Jakub's description of the problem I'm fairly certain that ld can produce a valid program. Apologies for not picking this up earlier when I reviewed your patch. bfd/ * merge.c (sec_merge_hash_lookup): Revert last change. (record_section): Likewise. (_bfd_merge_sections): Likewise. (_bfd_merged_section_offset): Properly handle NULL secinfo. (_bfd_add_merge_section): Allocate extra space for a zero terminator on SEC_STRINGS sections. ld/testsuite/ * ld-elf/merge3.d, ld-elf/merge3.s: Delete. Index: bfd/merge.c =================================================================== RCS file: /cvs/src/src/bfd/merge.c,v retrieving revision 1.34 diff -u -p -r1.34 merge.c --- bfd/merge.c 1 Nov 2007 11:45:20 -0000 1.34 +++ bfd/merge.c 5 Nov 2007 02:07:05 -0000 @@ -133,7 +133,6 @@ sec_merge_hash_newfunc (struct bfd_hash_ static struct sec_merge_hash_entry * sec_merge_hash_lookup (struct sec_merge_hash *table, const char *string, - const unsigned char *sec_end, unsigned int alignment, bfd_boolean create) { register const unsigned char *s; @@ -155,8 +154,6 @@ sec_merge_hash_lookup (struct sec_merge_ hash += c + (c << 17); hash ^= hash >> 2; ++len; - if (sec_end && s >= sec_end) - return NULL; } hash += len + (len << 17); } @@ -164,8 +161,6 @@ sec_merge_hash_lookup (struct sec_merge_ { for (;;) { - if (sec_end && s + table->entsize > sec_end) - return NULL; for (i = 0; i < table->entsize; ++i) if (s[i] != '\0') break; @@ -269,9 +264,7 @@ sec_merge_add (struct sec_merge_hash *ta { register struct sec_merge_hash_entry *entry; - entry = sec_merge_hash_lookup (tab, str, - secinfo->contents + secinfo->sec->size, - alignment, TRUE); + entry = sec_merge_hash_lookup (tab, str, alignment, TRUE); if (entry == NULL) return NULL; @@ -410,7 +403,12 @@ _bfd_add_merge_section (bfd *abfd, void /* Read the section from abfd. */ - amt = sizeof (struct sec_merge_sec_info) + sec->size - 1; + amt = sizeof (struct sec_merge_sec_info) - 1 + sec->size; + if (sec->flags & SEC_STRINGS) + /* Some versions of gcc may emit a string without a zero terminator. + See http://gcc.gnu.org/ml/gcc-patches/2006-06/msg01004.html + Allocate space for an extra zero. */ + amt += sec->entsize; *psecinfo = bfd_alloc (abfd, amt); if (*psecinfo == NULL) goto error_return; @@ -430,6 +428,8 @@ _bfd_add_merge_section (bfd *abfd, void secinfo->first_str = NULL; sec->rawsize = sec->size; + if (sec->flags & SEC_STRINGS) + memset (secinfo->contents + sec->size, 0, sec->entsize); if (! bfd_get_section_contents (sec->owner, sec, secinfo->contents, 0, sec->size)) goto error_return; @@ -443,8 +443,7 @@ _bfd_add_merge_section (bfd *abfd, void /* Record one section into the hash table. */ static bfd_boolean -record_section (bfd *abfd, - struct sec_merge_info *sinfo, +record_section (struct sec_merge_info *sinfo, struct sec_merge_sec_info *secinfo) { asection *sec = secinfo->sec; @@ -521,10 +520,6 @@ record_section (bfd *abfd, return TRUE; error_return: - if (bfd_get_error () != bfd_error_no_memory) - (*_bfd_error_handler) - (_("%B: unterminated string in section `%A' marked for merging"), - abfd, sec); for (secinfo = sinfo->chain; secinfo; secinfo = secinfo->next) *secinfo->psecinfo = NULL; return FALSE; @@ -734,7 +729,7 @@ _bfd_merge_sections (bfd *abfd, if (remove_hook) (*remove_hook) (abfd, secinfo->sec); } - else if (! record_section (abfd, sinfo, secinfo)) + else if (! record_section (sinfo, secinfo)) break; if (secinfo) @@ -823,7 +818,7 @@ _bfd_merged_section_offset (bfd *output_ secinfo = (struct sec_merge_sec_info *) psecinfo; if (!secinfo) - return 0; + return offset; if (offset >= sec->rawsize) { @@ -867,7 +862,7 @@ _bfd_merged_section_offset (bfd *output_ { p = secinfo->contents + (offset / sec->entsize) * sec->entsize; } - entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, NULL, 0, FALSE); + entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, 0, FALSE); if (!entry) { if (! secinfo->htab->strings) -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch to detect invalid mergeable string sections 2007-11-05 2:12 ` Alan Modra @ 2007-11-05 2:31 ` Joseph S. Myers 2007-11-05 3:59 ` Alan Modra 0 siblings, 1 reply; 7+ messages in thread From: Joseph S. Myers @ 2007-11-05 2:31 UTC (permalink / raw) To: Alan Modra; +Cc: binutils On Mon, 5 Nov 2007, Alan Modra wrote: > ld/testsuite/ > * ld-elf/merge3.d, ld-elf/merge3.s: Delete. It seems to me tests are still needed to make sure the linker doesn't crash (or indeed to verify that the unterminated strings now get properly merged). -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Patch to detect invalid mergeable string sections 2007-11-05 2:31 ` Joseph S. Myers @ 2007-11-05 3:59 ` Alan Modra 0 siblings, 0 replies; 7+ messages in thread From: Alan Modra @ 2007-11-05 3:59 UTC (permalink / raw) To: Joseph S. Myers; +Cc: binutils On Mon, Nov 05, 2007 at 02:31:18AM +0000, Joseph S. Myers wrote: > On Mon, 5 Nov 2007, Alan Modra wrote: > > > ld/testsuite/ > > * ld-elf/merge3.d, ld-elf/merge3.s: Delete. > > It seems to me tests are still needed to make sure the linker doesn't > crash (or indeed to verify that the unterminated strings now get properly > merged). That could be quite challenging. A meaningful test should fail on a 20071031 ld and pass with today's ld. I think you'd need to replace ld's memory allocation functions with ones that filled memory with some non-zero pattern, or run ld under valgrind or somesuch to reliably detect an error caused by buffer overrun. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2007-11-05 3:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-10-30 16:19 Patch to detect invalid mergeable string sections Joseph S. Myers 2007-10-30 23:39 ` Alan Modra 2007-10-31 18:17 ` Joseph S. Myers 2007-11-01 7:11 ` Alan Modra 2007-11-05 2:12 ` Alan Modra 2007-11-05 2:31 ` Joseph S. Myers 2007-11-05 3:59 ` Alan Modra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).