On 2026-01-05 11:55, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
>
> Simon> This is more C++-y. Remove the struct keywords from pop_context just to
> Simon> match.
>
> Simon> Rename "newobj" to "ctx" in the users of context_stack, because I think
> Simon> the "newobj" name is meaningless.
>
> Simon> For a later task: I think we should find a better name for
> Simon> context_stack, because it is not a stack (it is an entry in the context
> Simon> stack).
>
> Thanks, this seems fine to me.
>
> This made me look up m_context_stack and I see it is a std::vector. So
> I wonder if we ever have multiple entries on the stack, it seems like
> there's a possible UAF here if the stack is resized at the wrong moment.
> Perhaps it ought to be a linked list or deque or something instead.
It seems like it would be very easy for that to happen, but I didn't
spot an actual problem in the DWARF reader. In read_func_scope, we
push a context and grab a reference to it. But it looks like we don't
call anything that could recurse in the DIEs, by the time we are done
using it.
Likewise in coffread.c, function coff_symtab_read.
Still, some cleanups to do there.
Simon
@@ -170,14 +170,14 @@ end_compunit_symtab (CORE_ADDR end_addr)
return result;
}
-struct context_stack *
+context_stack &
push_context (int desc, CORE_ADDR valu)
{
gdb_assert (buildsym_compunit != nullptr);
return buildsym_compunit->push_context (desc, valu);
}
-struct context_stack
+context_stack
pop_context ()
{
gdb_assert (buildsym_compunit != nullptr);
@@ -82,9 +82,9 @@ extern const char *pop_subfile ();
extern struct compunit_symtab *end_compunit_symtab (CORE_ADDR end_addr);
-extern struct context_stack *push_context (int desc, CORE_ADDR valu);
+extern context_stack &push_context (int desc, CORE_ADDR valu);
-extern struct context_stack pop_context ();
+extern context_stack pop_context ();
extern void record_line (struct subfile *subfile, int line,
unrelocated_addr pc);
@@ -1061,32 +1061,32 @@ buildsym_compunit::augment_type_symtab ()
(checkable when you pop it), and the starting PC address of this
context. */
-struct context_stack *
+context_stack &
buildsym_compunit::push_context (int desc, CORE_ADDR valu)
{
- struct context_stack *newobj = &m_context_stack.emplace_back ();
+ context_stack &ctx = m_context_stack.emplace_back ();
- newobj->depth = desc;
- newobj->locals = m_local_symbols;
- newobj->old_blocks = m_pending_blocks;
- newobj->start_addr = valu;
- newobj->local_using_directives = m_local_using_directives;
- newobj->name = NULL;
+ ctx.depth = desc;
+ ctx.locals = m_local_symbols;
+ ctx.old_blocks = m_pending_blocks;
+ ctx.start_addr = valu;
+ ctx.local_using_directives = m_local_using_directives;
+ ctx.name = NULL;
m_local_symbols = NULL;
m_local_using_directives = NULL;
- return newobj;
+ return ctx;
}
/* Pop a context block. Returns the address of the context block just
popped. */
-struct context_stack
+context_stack
buildsym_compunit::pop_context ()
{
gdb_assert (!m_context_stack.empty ());
- struct context_stack result = m_context_stack.back ();
+ context_stack result = m_context_stack.back ();
m_context_stack.pop_back ();
return result;
}
@@ -306,9 +306,9 @@ struct buildsym_compunit
m_producer = producer;
}
- struct context_stack *push_context (int desc, CORE_ADDR valu);
+ context_stack &push_context (int desc, CORE_ADDR valu);
- struct context_stack pop_context ();
+ context_stack pop_context ();
struct block *end_compunit_symtab_get_static_block
(CORE_ADDR end_addr, bool expandable, bool required);
@@ -745,7 +745,7 @@ coff_symtab_read (minimal_symbol_reader &reader,
struct objfile *objfile)
{
struct gdbarch *gdbarch = objfile->arch ();
- struct context_stack *newobj = nullptr;
+ context_stack *ctx = nullptr;
struct coff_symbol coff_symbol;
struct coff_symbol *cs = &coff_symbol;
static struct internal_syment main_sym;
@@ -1029,11 +1029,10 @@ coff_symtab_read (minimal_symbol_reader &reader,
context_stack_depth is zero, and complain if not. */
depth = 0;
- newobj = push_context (depth, fcn_start_addr);
+ ctx = &push_context (depth, fcn_start_addr);
fcn_cs_saved.c_name = getsymname (&fcn_sym_saved);
- newobj->name =
- process_coff_symbol (&fcn_cs_saved,
- &fcn_aux_saved, objfile);
+ ctx->name
+ = process_coff_symbol (&fcn_cs_saved, &fcn_aux_saved, objfile);
}
else if (strcmp (cs->c_name, ".ef") == 0)
{
@@ -1055,7 +1054,7 @@ coff_symtab_read (minimal_symbol_reader &reader,
struct context_stack cstk = pop_context ();
/* Stack must be empty now. */
- if (!outermost_context_p () || newobj == NULL)
+ if (!outermost_context_p () || ctx == nullptr)
{
complaint (_("Unmatched .ef symbol(s) ignored "
"starting at symnum %d"),
@@ -8505,7 +8505,6 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
dwarf2_per_objfile *per_objfile = cu->per_objfile;
struct objfile *objfile = per_objfile->objfile;
struct gdbarch *gdbarch = objfile->arch ();
- struct context_stack *newobj;
CORE_ADDR lowpc;
CORE_ADDR highpc;
struct attribute *attr, *call_line, *call_file;
@@ -8596,27 +8595,27 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
}
gdb_assert (cu->get_builder () != nullptr);
- newobj = cu->get_builder ()->push_context (0, lowpc);
- newobj->name = new_symbol (die, read_type_die (die, cu), cu, templ_func);
+ context_stack &ctx = cu->get_builder ()->push_context (0, lowpc);
+ ctx.name = new_symbol (die, read_type_die (die, cu), cu, templ_func);
if (dwarf2_func_is_main_p (die, cu))
- set_objfile_main_name (objfile, newobj->name->linkage_name (),
+ set_objfile_main_name (objfile, ctx.name->linkage_name (),
cu->lang ());
/* If there is a location expression for DW_AT_frame_base, record
it. */
attr = dwarf2_attr (die, DW_AT_frame_base, cu);
if (attr != nullptr)
- dwarf2_symbol_mark_computed (attr, newobj->name, cu, 1);
+ dwarf2_symbol_mark_computed (attr, ctx.name, cu, 1);
/* If there is a location for the static link, record it. */
- newobj->static_link = NULL;
+ ctx.static_link = NULL;
attr = dwarf2_attr (die, DW_AT_static_link, cu);
if (attr != nullptr)
{
- newobj->static_link
+ ctx.static_link
= XOBNEW (&objfile->objfile_obstack, struct dynamic_prop);
- attr_to_dynamic_prop (attr, die, cu, newobj->static_link,
+ attr_to_dynamic_prop (attr, die, cu, ctx.static_link,
cu->addr_type ());
}