Discussion:
[PATCH-bfd] i386-mingw32-ld crash on x86_64 linux
(too old to reply)
Peter O'Gorman
2009-04-14 14:05:49 UTC
Permalink
Hi,

We had a customer report a ld crash on an x86_64 linux cross compiler
setup targetting mingw32, with some of the input libraries generated by
visual studio, some generated by the GNU tools. They had no issues with
a similar setup on i386 linux.

The crash occurred with binutils-2.18.50 and 2.19.1 here:

bfd_vma sec_vma = s->output_section->vma + s->output_offset;

#0 0x0000000000426ef8 in generate_reloc (abfd=0x618fc0, info=0x60a140) at ../../ld/pe-dll.c:1258
#1 0x000000000042b0c6 in pe_dll_fill_sections (abfd=0x618fc0, info=0x60a140) at ../../ld/pe-dll.c:2887
#2 0x000000000042230a in gld_i386pe_finish () at ei386pe.c:1540
#3 0x000000000041c5e4 in ldemul_finish () at ../../ld/ldemul.c:90
#4 0x0000000000414306 in lang_process () at ../../ld/ldlang.c:6186
#5 0x0000000000417bb1 in main (argc=35, argv=0x7fbfffbf38) at ../../ld/ldmain.c:453

s->output_section is NULL here.

gdb tells us that the bfd in question has the following sections:
.debug$S
.idata$2
.idata$6
.idata$4\004
.idata$5\004
.idata$4
.idata$5

This library was generated by Visual Studio 2005 SP1.

As you can see there is an issue, there are two instances of .idata$4
and 5, one with a trailing 0x04, one without. One has output_section as
a valid section, one is NULL.

This patch fixes the crash, though we are still unsure why it crashes
with an x86_64 linux build and works with an i386 linux build.

2009-04-14 Peter O'Gorman <***@thewrittenword.com>

* peXXigen.c: Ensure in->_n._n_name is NULL terminated.

Thanks,
Peter
--
Peter O'Gorman
***@thewrittenword.com
Dave Korn
2009-04-14 14:38:40 UTC
Permalink
Post by Peter O'Gorman
This patch fixes the crash, though we are still unsure why it crashes
with an x86_64 linux build and works with an i386 linux build.
Different 32-vs-64 type sizes or memory map layouts, who knows.
Post by Peter O'Gorman
* peXXigen.c: Ensure in->_n._n_name is NULL terminated.
This looks wrong, for two reasons:

1. Buffer overflow: this writes one byte past the end of the _n_name array,
potentially corrupting the n_value field.

+ in->_n._n_name[SYMNMLEN] = 0;

2. Disregards the PE specification: from chapter 4 (Section table):

Offset Size Field Description
0 8 Name An 8-byte, null-padded UTF-8 encoded string. If the string is
exactly 8 characters long, there is no terminating null. For longer names,
this field contains a slash (/) that is followed by an ASCII representation of
a decimal number that is an offset into the string table. Executable images do
not use a string table and do not support section names longer than 8
characters. Long names in object files are truncated if they are emitted to an
executable file.

I think you probably need to reanalyze what's going on here. The names
aren't supposed to be nul-terminated, but we don't know for sure if gdb is
displaying them correctly; those ones with the trailing \004 are too long to
fit in the _n_name field, so ought to be stored as long section names in the
COFF string table. The other possibility is that GDB has displayed a byte too
many because it has a bug and expects nul-terminated strings, the \004 being
the low byte of n_value. If that turns out to be the case, then the
.idata$4/.idata$5 section names are actually duplicated, which could possibly
explain the problem.

Got a simple testcase? Are you allowed to share the .o member from the .lib
file involved?

cheers,
DaveK
Peter O'Gorman
2009-04-14 15:37:08 UTC
Permalink
Post by Dave Korn
Post by Peter O'Gorman
This patch fixes the crash, though we are still unsure why it crashes
with an x86_64 linux build and works with an i386 linux build.
Different 32-vs-64 type sizes or memory map layouts, who knows.
Post by Peter O'Gorman
* peXXigen.c: Ensure in->_n._n_name is NULL terminated.
1. Buffer overflow: this writes one byte past the end of the _n_name array,
potentially corrupting the n_value field.
Ugh :(
Post by Dave Korn
+ in->_n._n_name[SYMNMLEN] = 0;
Offset Size Field Description
0 8 Name An 8-byte, null-padded UTF-8 encoded string. If the string is
exactly 8 characters long, there is no terminating null. For longer names,
this field contains a slash (/) that is followed by an ASCII representation of
a decimal number that is an offset into the string table. Executable images do
not use a string table and do not support section names longer than 8
characters. Long names in object files are truncated if they are emitted to an
executable file.
Later in the same function there is:
if (strcmp (sec->name, in->n_name) == 0)

So that would need to change to strncmp, with luck that is the only
place that expects n_name to be null terminated.
Post by Dave Korn
I think you probably need to reanalyze what's going on here. The names
aren't supposed to be nul-terminated, but we don't know for sure if gdb is
displaying them correctly; those ones with the trailing \004 are too long to
fit in the _n_name field, so ought to be stored as long section names in the
COFF string table. The other possibility is that GDB has displayed a byte too
many because it has a bug and expects nul-terminated strings, the \004 being
the low byte of n_value. If that turns out to be the case, then the
.idata$4/.idata$5 section names are actually duplicated, which could possibly
explain the problem.
Will look into this.
Post by Dave Korn
Got a simple testcase?
No, it only fails for this one library.
Post by Dave Korn
Are you allowed to share the .o member from the .lib
file involved?
Afraid not.

Thanks for the reply, was extremely helpful.

Peter
--
Peter O'Gorman
***@thewrittenword.com
Dave Korn
2009-04-14 17:43:32 UTC
Permalink
Later in the same function there is: if (strcmp (sec->name, in->n_name) ==
0)
So that would need to change to strncmp, with luck that is the only place
that expects n_name to be null terminated.
Ouch, looks like you've opened a can of worms here, I've spotted a couple
more already. I'll have a good read through the file.

cheers,
DaveK
Peter O'Gorman
2009-04-14 17:51:15 UTC
Permalink
Post by Dave Korn
Later in the same function there is: if (strcmp (sec->name, in->n_name) ==
0)
So that would need to change to strncmp, with luck that is the only place
that expects n_name to be null terminated.
Ouch, looks like you've opened a can of worms here, I've spotted a couple
more already. I'll have a good read through the file.
Simply changing that one strcmp to strncmp eliminates the crash, but the
output differs from the same link done on i686 linux (we don't have
source code, or a way to test the output, so cmp is the only check we
have).

On x86_64 the union is 16 bytes long, because bfd_hostptr_t is an
unsigned long. If we zero the _n_zeroes and _n_offset members before we
do the memcpy, then the output on x86_64 is identical to that on i686.

Is this patch more acceptable?

Thank you,
Peter
--
Peter O'Gorman
***@thewrittenword.com
Dave Korn
2009-04-16 04:16:24 UTC
Permalink
Post by Peter O'Gorman
Simply changing that one strcmp to strncmp eliminates the crash, but the
output differs from the same link done on i686 linux (we don't have
source code, or a way to test the output, so cmp is the only check we
have).
Um, isn't objdump useful for this? And LD's "-Map" option? That's how I
generally try and track down unexplained linking differences. Source code
isn't necessary, it won't tell you anything about what the linker's doing
differently anyway.
Post by Peter O'Gorman
On x86_64 the union is 16 bytes long, because bfd_hostptr_t is an
unsigned long. If we zero the _n_zeroes and _n_offset members before we
do the memcpy, then the output on x86_64 is identical to that on i686.
Is this patch more acceptable?
Well, the strcmp->strncmp bit is correct. The changes to zero out _n_zeroes
and _n_offsets... no, not really. I mean, they're not wrong, but I don't
think we should check in patches that we don't know how or why they work; that
way cargo-cult programming lies. If the code requires more than one member of
a *union* to be initialised in order to work correctly, that means that
somewhere down the line we're probably actually doing something undefined or
invalid when it comes to accessing that union.

Also there shouldn't have been any need to initialise _n_zeroes, only
_n_offset, because _n_zeros gets overwritten by the memcpy. Since the section
names are eight chars, it must be the _n_offset field that is somehow making
the difference.

We really do need to understand what is different better. Can you try,
instead of zeroing the n_offset field, write a recognisable pattern into it
and see if/where it turns up later in the final linked exe? Meanwhile, I'm
going to go through the sources looking at all the references to _n_offset and
see if I can figure out where it's coming back into relevance. It might even
be that zeroing it actually is the correct thing to do, but we don't know yet.


cheers,
DaveK
Dave Korn
2009-04-16 13:29:26 UTC
Permalink
Post by Dave Korn
Post by Peter O'Gorman
Is this patch more acceptable?
Well, the strcmp->strncmp bit is correct. The changes to zero out _n_zeroes
and _n_offsets... no, not really. I mean, they're not wrong, but I don't
think we should check in patches that we don't know how or why they work.
However it might be a perfectly practical stopgap solution for your customer
who reported the issue, I was just talking about what we should do in binutils
CVS.

cheers,
DaveK
Peter O'Gorman
2009-04-17 02:41:35 UTC
Permalink
Post by Dave Korn
Well, the strcmp->strncmp bit is correct. The changes to zero out _n_zeroes
and _n_offsets... no, not really. I mean, they're not wrong, but I don't
think we should check in patches that we don't know how or why they work; that
way cargo-cult programming lies. If the code requires more than one member of
a *union* to be initialised in order to work correctly, that means that
somewhere down the line we're probably actually doing something undefined or
invalid when it comes to accessing that union.
Thanks for pushing on this, I was tired of looking at ld in gdb, and
wanted it to stop :)

As for using objdump, on the complete test case from the customer, I
get:
% /opt/build/binutils-2.19.1/build-binutils/binutils/objdump -s __test.a
BFD: BFD (GNU Binutils) 2.19.1 internal error, aborting at ../../bfd/coffcode.h line 842 in handle_COMDAT

BFD: Please report this bug.

This version of coffcodegen.h has:
if (! (isym.n_sclass == C_STAT
&& isym.n_type == T_NULL
&& isym.n_value == 0))
abort ();

I guess you'll want more details on that too, please let me know what
you need.

Anyway, fixing the next couple of cases of expecting _n_name to be null
terminated fixes the crash, and the output no longer contains the
'0xdabedabe' sequence that I tested with.

It looks like this code does not handle the case where the section name
is longer than 8 bytes and is some kind of ascii representation of a
decimal offset into the string table?

Anyway, crash is gone, and I no longer feel like a cargo-cult
programmer!

Thanks very much!
Peter
--
Peter O'Gorman
***@thewrittenword.com
Alan Modra
2009-04-17 04:05:42 UTC
Permalink
Index: bfd/peXXigen.c
===================================================================
--- bfd/peXXigen.c.orig 2008-07-30 04:34:56.000000000 +0000
+++ bfd/peXXigen.c 2009-04-17 02:19:12.221139740 +0000
@@ -138,7 +138,7 @@
for (sec = abfd->sections; sec; sec = sec->next)
{
- if (strcmp (sec->name, in->n_name) == 0)
+ if (strncmp (sec->name, in->n_name, SYMNMLEN) == 0)
This isn't quite right. If in->n_name is not 0 terminated then you
could match the wrong section.
{
in->n_scnum = sec->target_index;
break;
@@ -157,10 +157,10 @@
if (unused_section_number <= sec->target_index)
unused_section_number = sec->target_index + 1;
- name = bfd_alloc (abfd, (bfd_size_type) strlen (in->n_name) + 10);
+ name = bfd_alloc (abfd, (bfd_size_type) SYMNMLEN + 1);
Better to use char[SYMNMLEN + 1] rather than bfd_alloc.
if (name == NULL)
return;
- strcpy (name, in->n_name);
+ strncpy (name, in->n_name, SYMNMLEN);
This also isn't quite correct. You want

strncpy (name, in->n_name, SYMNMLEN);
name[SYMNMLEN] = '\0';

So, something like the following. I won't commit this as it's
untested, and I'm not sure whether the ext->e.e_name[0] == 0 case
ought to be handled.

Index: bfd/peXXigen.c
===================================================================
RCS file: /cvs/src/src/bfd/peXXigen.c,v
retrieving revision 1.49
diff -u -p -r1.49 peXXigen.c
--- bfd/peXXigen.c 6 Apr 2009 16:48:36 -0000 1.49
+++ bfd/peXXigen.c 17 Apr 2009 03:59:32 -0000
@@ -129,6 +129,8 @@ _bfd_XXi_swap_sym_in (bfd * abfd, void *
they will be handled somewhat correctly in the bfd code. */
if (in->n_sclass == C_SECTION)
{
+ char name[SYMNMLEN + 1];
+
in->n_value = 0x0;

/* Create synthetic empty sections as needed. DJ */
@@ -136,31 +138,23 @@ _bfd_XXi_swap_sym_in (bfd * abfd, void *
{
asection *sec;

- for (sec = abfd->sections; sec; sec = sec->next)
- {
- if (strcmp (sec->name, in->n_name) == 0)
- {
- in->n_scnum = sec->target_index;
- break;
- }
- }
+ strncpy (name, in->n_name, SYMNMLEN);
+ name[SYMNMLEN] = '\0';
+ sec = bfd_get_section_by_name (abfd, name);
+ if (sec != NULL)
+ in->n_scnum = sec->target_index;
}

if (in->n_scnum == 0)
{
int unused_section_number = 0;
asection *sec;
- char *name;
flagword flags;

for (sec = abfd->sections; sec; sec = sec->next)
if (unused_section_number <= sec->target_index)
unused_section_number = sec->target_index + 1;

- name = bfd_alloc (abfd, (bfd_size_type) strlen (in->n_name) + 10);
- if (name == NULL)
- return;
- strcpy (name, in->n_name);
flags = SEC_HAS_CONTENTS | SEC_ALLOC | SEC_DATA | SEC_LOAD;
sec = bfd_make_section_anyway_with_flags (abfd, name, flags);
--
Alan Modra
Australia Development Lab, IBM
Dave Korn
2009-04-17 12:11:31 UTC
Permalink
Post by Alan Modra
So, something like the following. I won't commit this as it's
untested, and I'm not sure whether the ext->e.e_name[0] == 0 case
ought to be handled.
Hmm. That case will arise if the symbol name is long, and I guess it could
be since we do allow long section names. I'll whip up something based on your
patch and run my standard set of testsuites on it.

Thanks Alan, and Pete for persisting with this.

cheers,
DaveK
Dave Korn
2009-04-18 14:54:06 UTC
Permalink
Post by Dave Korn
Post by Alan Modra
So, something like the following. I won't commit this as it's
untested, and I'm not sure whether the ext->e.e_name[0] == 0 case
ought to be handled.
Hmm. That case will arise if the symbol name is long, and I guess it could
be since we do allow long section names. I'll whip up something based on your
patch and run my standard set of testsuites on it.
I remembered that we had a function to handle all the name-related
complications for us already, so I just used it. It should be OK to call from
within _bfd_XXi_swap_sym_in, since it is often called immediately afterward in
callers of _bfd_XXi_swap_sym_in anyway.

I'm running this through a test cycle. Peter, would you care to test if it
fixes your specific problem?

bfd/ChangeLog

* peXXigen.c (_bfd_XXi_swap_sym_in): Fix name handling w.r.t
long names and non-NUL-terminated strings.

cheers,
DaveK
Peter O'Gorman
2009-04-18 15:47:19 UTC
Permalink
Post by Dave Korn
Post by Dave Korn
Post by Alan Modra
So, something like the following. I won't commit this as it's
untested, and I'm not sure whether the ext->e.e_name[0] == 0 case
ought to be handled.
Hmm. That case will arise if the symbol name is long, and I guess it could
be since we do allow long section names. I'll whip up something based on your
patch and run my standard set of testsuites on it.
I remembered that we had a function to handle all the name-related
complications for us already, so I just used it. It should be OK to call from
within _bfd_XXi_swap_sym_in, since it is often called immediately afterward in
callers of _bfd_XXi_swap_sym_in anyway.
I'm running this through a test cycle. Peter, would you care to test if it
fixes your specific problem?
Hi Dave,

Thanks for all the time you're spending on this.

With this patch, ld still core dumps (same place, x86_64), don't have
time to look into it right now, but will check more later on today.

Peter
--
Peter O'Gorman
***@thewrittenword.com
Dave Korn
2009-04-18 17:16:10 UTC
Permalink
Post by Peter O'Gorman
Thanks for all the time you're spending on this.
Likewise.
Post by Peter O'Gorman
With this patch, ld still core dumps (same place, x86_64), don't have
time to look into it right now, but will check more later on today.
Bah, sorry about that. Thanks for the bad news; I'll do some more digging.

cheers,
DaveK
Dave Korn
2009-04-18 17:47:07 UTC
Permalink
Post by Alan Modra
@@ -157,10 +157,10 @@
if (unused_section_number <= sec->target_index)
unused_section_number = sec->target_index + 1;
- name = bfd_alloc (abfd, (bfd_size_type) strlen (in->n_name) + 10);
+ name = bfd_alloc (abfd, (bfd_size_type) SYMNMLEN + 1);
Better to use char[SYMNMLEN + 1] rather than bfd_alloc.
- name = bfd_alloc (abfd, (bfd_size_type) strlen (in->n_name) + 10);
- if (name == NULL)
- return;
- strcpy (name, in->n_name);
flags = SEC_HAS_CONTENTS | SEC_ALLOC | SEC_DATA | SEC_LOAD;
sec = bfd_make_section_anyway_with_flags (abfd, name, flags);
I'm not sure that bfd_make_section_anyway_with_flags copies the name. I
think it stashes the value of the pointer passed in (twice in fact, once in
the asection itself and once in the root bfd_hash_entry to which the section
is anchored), so we do need to allocate some memory after all. So I'll try
re-adding that and we'll see if it helps.

cheers,
DaveK
Peter O'Gorman
2009-04-19 03:34:16 UTC
Permalink
Post by Dave Korn
I'm not sure that bfd_make_section_anyway_with_flags copies the name. I
think it stashes the value of the pointer passed in (twice in fact, once in
the asection itself and once in the root bfd_hash_entry to which the section
is anchored), so we do need to allocate some memory after all. So I'll try
re-adding that and we'll see if it helps.
Yes, that seems to be it! If I add a bfd_alloc and strcpy, it works.

Attached is what I used to test (your patch + alloc & copy).

Thank you!

Peter
--
Peter O'Gorman
***@thewrittenword.com
Dave Korn
2009-04-19 12:04:30 UTC
Permalink
Post by Peter O'Gorman
Post by Dave Korn
I'm not sure that bfd_make_section_anyway_with_flags copies the name. I
think it stashes the value of the pointer passed in (twice in fact, once in
the asection itself and once in the root bfd_hash_entry to which the section
is anchored), so we do need to allocate some memory after all. So I'll try
re-adding that and we'll see if it helps.
Yes, that seems to be it! If I add a bfd_alloc and strcpy, it works.
Attached is what I used to test (your patch + alloc & copy).
Thank you!
Brilliant. I'll run it through a full testsuite and we'll see about getting
it checked in. Thanks for contributing :)

cheers,
DaveK
Dave Korn
2009-04-19 21:30:41 UTC
Permalink
Post by Dave Korn
Brilliant. I'll run it through a full testsuite and we'll see about getting
it checked in. Thanks for contributing :)
Tested on i686-pc-cygwin natively, and cross from i686-pc-linux-gnu to
{arm-epoc-pe, arm-wince-pe, i386-pc-netbsdpe, i386-pc-pe, i586-pc-interix,
i586-unknown-beospe, i686-pc-cygwin, i686-pc-mingw32, mcore-unknown-pe,
powerpcle-unknown-pe, sh-unknown-pe, thumb-epoc-pe, x86_64-pc-freebsd,
x86_64-pc-linux-gnu, x86_64-pc-mingw32} without regressions.

bfd/ChangeLog

2009-04-19 Peter O'Gorman <***@mlists.thewrittenword.com>
Alan Modra <***@bigpond.net.au>
Dave Korn <***@gmail.com>

* peXXigen.c (_bfd_XXi_swap_sym_in): Fix name handling w.r.t
long names and non-NUL-terminated strings.

Ok?

cheers,
DaveK
H.J. Lu
2009-04-19 22:43:15 UTC
Permalink
On Sun, Apr 19, 2009 at 2:30 PM, Dave Korn
  Brilliant.  I'll run it through a full testsuite and we'll see about getting
it checked in.  Thanks for contributing :)
 Tested on i686-pc-cygwin natively, and cross from i686-pc-linux-gnu to
{arm-epoc-pe, arm-wince-pe, i386-pc-netbsdpe, i386-pc-pe, i586-pc-interix,
i586-unknown-beospe, i686-pc-cygwin, i686-pc-mingw32, mcore-unknown-pe,
powerpcle-unknown-pe, sh-unknown-pe, thumb-epoc-pe, x86_64-pc-freebsd,
x86_64-pc-linux-gnu, x86_64-pc-mingw32} without regressions.
bfd/ChangeLog
       * peXXigen.c (_bfd_XXi_swap_sym_in):  Fix name handling w.r.t
       long names and non-NUL-terminated strings.
 Ok?
Where is the patch?
--
H.J.
Dave Korn
2009-04-20 01:05:39 UTC
Permalink
Post by H.J. Lu
On Sun, Apr 19, 2009 at 2:30 PM, Dave Korn
Post by Dave Korn
Post by Dave Korn
Brilliant. I'll run it through a full testsuite and we'll see about getting
it checked in. Thanks for contributing :)
Tested on i686-pc-cygwin natively, and cross from i686-pc-linux-gnu to
{arm-epoc-pe, arm-wince-pe, i386-pc-netbsdpe, i386-pc-pe, i586-pc-interix,
i586-unknown-beospe, i686-pc-cygwin, i686-pc-mingw32, mcore-unknown-pe,
powerpcle-unknown-pe, sh-unknown-pe, thumb-epoc-pe, x86_64-pc-freebsd,
x86_64-pc-linux-gnu, x86_64-pc-mingw32} without regressions.
bfd/ChangeLog
* peXXigen.c (_bfd_XXi_swap_sym_in): Fix name handling w.r.t
long names and non-NUL-terminated strings.
Ok?
Where is the patch?
Oops. Here is what I tested; it differs from Peter's original only in two
insignificant whitespaces.

cheers,
DaveK
Alan Modra
2009-04-20 01:09:59 UTC
Permalink
Post by Dave Korn
Post by Dave Korn
Brilliant. I'll run it through a full testsuite and we'll see about getting
it checked in. Thanks for contributing :)
Tested on i686-pc-cygwin natively, and cross from i686-pc-linux-gnu to
{arm-epoc-pe, arm-wince-pe, i386-pc-netbsdpe, i386-pc-pe, i586-pc-interix,
i586-unknown-beospe, i686-pc-cygwin, i686-pc-mingw32, mcore-unknown-pe,
powerpcle-unknown-pe, sh-unknown-pe, thumb-epoc-pe, x86_64-pc-freebsd,
x86_64-pc-linux-gnu, x86_64-pc-mingw32} without regressions.
bfd/ChangeLog
* peXXigen.c (_bfd_XXi_swap_sym_in): Fix name handling w.r.t
long names and non-NUL-terminated strings.
This is what I've committed. Adds some aborts on the grounds that an
abort is better than a segfault. Since other parts of the coff
support do the same I don't feel particularly guilty about an abort
that could be triggered by bad user input or out of memory. A proper
fix requires quite a lot of surgery.

Index: bfd/peXXigen.c
===================================================================
RCS file: /cvs/src/src/bfd/peXXigen.c,v
retrieving revision 1.49
diff -u -p -r1.49 peXXigen.c
--- bfd/peXXigen.c 6 Apr 2009 16:48:36 -0000 1.49
+++ bfd/peXXigen.c 20 Apr 2009 00:09:39 -0000
@@ -129,6 +129,9 @@ _bfd_XXi_swap_sym_in (bfd * abfd, void *
they will be handled somewhat correctly in the bfd code. */
if (in->n_sclass == C_SECTION)
{
+ char namebuf[SYMNMLEN + 1];
+ const char *name;
+
in->n_value = 0x0;

/* Create synthetic empty sections as needed. DJ */
@@ -136,33 +139,38 @@ _bfd_XXi_swap_sym_in (bfd * abfd, void *
{
asection *sec;

- for (sec = abfd->sections; sec; sec = sec->next)
- {
- if (strcmp (sec->name, in->n_name) == 0)
- {
- in->n_scnum = sec->target_index;
- break;
- }
- }
+ name = _bfd_coff_internal_syment_name (abfd, in, namebuf);
+ if (name == NULL)
+ /* FIXME: Return error. */
+ abort ();
+ sec = bfd_get_section_by_name (abfd, name);
+ if (sec != NULL)
+ in->n_scnum = sec->target_index;
}

if (in->n_scnum == 0)
{
int unused_section_number = 0;
asection *sec;
- char *name;
flagword flags;

for (sec = abfd->sections; sec; sec = sec->next)
if (unused_section_number <= sec->target_index)
unused_section_number = sec->target_index + 1;

- name = bfd_alloc (abfd, (bfd_size_type) strlen (in->n_name) + 10);
- if (name == NULL)
- return;
- strcpy (name, in->n_name);
+ if (name == namebuf)
+ {
+ name = bfd_alloc (abfd, strlen (namebuf) + 1);
+ if (name == NULL)
+ /* FIXME: Return error. */
+ abort ();
+ strcpy ((char *) name, namebuf);
+ }
flags = SEC_HAS_CONTENTS | SEC_ALLOC | SEC_DATA | SEC_LOAD;
sec = bfd_make_section_anyway_with_flags (abfd, name, flags);
+ if (sec == NULL)
+ /* FIXME: Return error. */
+ abort ();

sec->vma = 0;
sec->lma = 0;
--
Alan Modra
Australia Development Lab, IBM
Dave Korn
2009-04-20 02:20:28 UTC
Permalink
Post by Alan Modra
Adds some aborts on the grounds that an
abort is better than a segfault. Since other parts of the coff
support do the same I don't feel particularly guilty about an abort
that could be triggered by bad user input or out of memory.
Nah, nor do I. If any of those conditions fire, you're already having a
really bad day anyway! Thanks for checking it in.

cheers,
DaveK

Loading...