Re: [patch] Set bfd field in target_section

Pedro Alves
2009-07-28T15:57:42Z

On Tuesday 28 July 2009 16:16:23, Aleksandar Ristovski wrote:
> Pedro Alves wrote:
> I think now you broke it in a different way. Now we can end
> up trying to read from a closed bfd.

Ooops, you're right. There are uses of the bfd after transfering
its ownership to the bfd target with target_bfd_reopen, and closing
the bfd target (target_close)... This target is modeled on fdopen,
which is why I blindly assumed it wasn't happening. I've reverted
that patch.

> And just wondering, why not simply:
>
> Index: gdb/exec.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/exec.c,v
> retrieving revision 1.90
> diff -u -p -r1.90 exec.c
> --- gdb/exec.c 2 Jul 2009 17:21:06 -0000 1.90
> +++ gdb/exec.c 28 Jul 2009 14:58:16 -0000
> @@ -381,6 +381,7 @@ add_to_section_table (bfd *abfd, struct
> struct target_section **table_pp = (struct
> target_section **) table_pp_char;
> flagword aflag;
>
> + (*table_pp)->bfd = abfd;
> /* Check the section flags, but do not discard
> zero-length sections, since
> some symbols may still be attached to this section.
> For instance, we
> encountered on sparc-solaris 2.10 a shared library
> with an empty .bss
> @@ -390,7 +391,6 @@ add_to_section_table (bfd *abfd, struct
> if (!(aflag & SEC_ALLOC))
> return;
>
> - (*table_pp)->bfd = abfd;
> (*table_pp)->the_bfd_section = asect;
> (*table_pp)->addr = bfd_section_vma (abfd, asect);
> (*table_pp)->endaddr = (*table_pp)->addr +
> bfd_section_size (abfd, asect);

Still not right, because that's still papering over the problem, and
still hitting undefined behaviour, in the case of the
table ending up being empty. When table->sections == table->sections_end,
we should never dereference table->sections, like in 'table->sections->bfd'.
Notice how the end pointer isn't advanced if !SEC_ALLOC.
Consider the fact that we usually xmalloc enough memory that
makes that not misbehave a coincidence. E.g., see that with
a resize_section_table call, you can end up with table->sections == NULL,
and table->section_end == NULL, which is legal, because table->sections
is still equal to table->sections_end in that case (meaning empty table).

There used to be an extra structure in bfd_target.c to hold
the sections and the bfd, which didn't look necessary when I
reimplemented bfd-target.c on top of exec.c's functions, but
it is clear now that we need it anyway. Here's a patch that
brings something like that back.

Could you try it? Thanks.

--
Pedro Alves

2009-07-28 Pedro Alves <pedro@codesourcery.com>

* bfd-target.c (struct target_bfd_data): New.
(target_bfd_xfer_partial): Adjust to get at the section table from
the new structure.
(target_bfd_get_section_table): Ditto.
(target_bfd_xclose): Ditto. Get the bfd pointer from the
target_bfd_data structure, from the section table.
(target_bfd_reopen): Store a struct target_bfd_data in the
target_ops to_data field, instead of a target_section_table.

---
gdb/bfd-target.c | 41 ++++++++++++++++++++++++++++-------------
1 file changed, 28 insertions(+), 13 deletions(-)

Index: src/gdb/bfd-target.c
===================================================================
--- src.orig/gdb/bfd-target.c 2009-07-28 16:20:59.000000000 +0100
+++ src/gdb/bfd-target.c 2009-07-28 16:48:12.000000000 +0100
@@ -22,6 +22,19 @@
#include "bfd-target.h"
#include "exec.h"

+/* The object that is stored in the target_ops->to_data field has this
+ type. */
+struct target_bfd_data
+{
+ /* The BFD we're wrapping. */
+ struct bfd *bfd;
+
+ /* The section table build from the ALLOC sections in BFD. Note
+ that we rely on extracting the BFD from a random section in the
+ table, since the table can be legitimately empty. */
+ struct target_section_table table;
+};
+
static LONGEST
target_bfd_xfer_partial (struct target_ops *ops,
enum target_object object,
@@ -33,10 +46,10 @@ target_bfd_xfer_partial (struct target_o
{
case TARGET_OBJECT_MEMORY:
{
- struct target_section_table *table = ops->to_data;
+ struct target_bfd_data *data = ops->to_data;
return section_table_xfer_memory_partial (readbuf, writebuf, offset, len,
- table->sections,
- table->sections_end,
+ data->table.sections,
+ data->table.sections_end,
NULL);
}
default:
@@ -47,17 +60,18 @@ target_bfd_xfer_partial (struct target_o
static struct target_section_table *
target_bfd_get_section_table (struct target_ops *ops)
{
- return ops->to_data;
+ struct target_bfd_data *data = ops->to_data;
+ return &data->table;
}

static void
target_bfd_xclose (struct target_ops *t, int quitting)
{
- struct target_section_table *table = t->to_data;
- if (table->sections)
- bfd_close (table->sections->bfd);
- xfree (table->sections);
- xfree (table);
+ struct target_bfd_data *data = t->to_data;
+
+ bfd_close (data->bfd);
+ xfree (data->table.sections);
+ xfree (data);
xfree (t);
}

@@ -65,10 +79,11 @@ struct target_ops *
target_bfd_reopen (struct bfd *bfd)
{
struct target_ops *t;
- struct target_section_table *table;
+ struct target_bfd_data *data;

- table = XZALLOC (struct target_section_table);
- build_section_table (bfd, &table->sections, &table->sections_end);
+ data = XZALLOC (struct target_bfd_data);
+ data->bfd = bfd;
+ build_section_table (bfd, &data->table.sections, &data->table.sections_end);

t = XZALLOC (struct target_ops);
t->to_shortname = "bfd";
@@ -77,7 +92,7 @@ target_bfd_reopen (struct bfd *bfd)
t->to_get_section_table = target_bfd_get_section_table;
t->to_xfer_partial = target_bfd_xfer_partial;
t->to_xclose = target_bfd_xclose;
- t->to_data = table;
+ t->to_data = data;

return t;
}