[PATCH] objdump: avoid possible SIGSEGV when printing debug info [BZ #32829]

[Alan Modra]

On Tue, Apr 01, 2025 at 10:33:26AM +0200, Jan Beulich wrote:
> On 31.03.2025 06:29, Collin Funk wrote:
> > * binutils/debug.c (debug_type_samep): Check for enum types where names
> > and values cannot be found.
> > ---
> >  binutils/debug.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> Some additional information might have been nice to provide here. From the
> bugzilla entry I get the feeling we're dealing with fuzzed input? That's
> relevant to at least mention. Further ...
> 
> > --- a/binutils/debug.c
> > +++ b/binutils/debug.c
> > @@ -3101,6 +3101,11 @@ debug_type_samep (struct debug_handle *info, struct debug_type_s *t1,
> >  	ret = t2->u.kenum == NULL;
> >        else if (t2->u.kenum == NULL)
> >  	ret = false;
> > +      else if (t1->u.kenum->names == NULL)
> > +	ret = t2->u.kenum->names == NULL
> > +	  && t1->u.kenum->values == t2->u.kenum->values;
> > +      else if (t2->u.kenum->names == NULL)
> > +	ret = false;
> 
> ... conditions under which ->names may be NULL would have been helpful to
> point out. The understanding I gained is this can (presently) only happen
> when an enumeration is installed by parse_coff_base_type() when no
> auxiliary symbol table entry is present.

Yes.

> That then raises the question whether - to not leave a similar trap in -
> ->u.kenum->values being NULL when ->u.kenum->names isn't shouldn't be
> dealt with as well.
> 
> I'd further raise the question whether two enums with no names/values
> ought to really be treated as being the same. Imo it's more likely that
> they're different (and there's no way to know for sure).

I'm inclined to not change the logic.  debug_type_samep is only used
in debug_set_class_id or recursively from that call.

> In summary - imo if any of the four pointers is NULL, we'd better return
> false here.

I didn't see Collin's patch before I wrote my own fix (which at first
looked just like Collin's).  I had considered whether it was necessary
to worry about all the pointers, but decided that the code testing for
NULL u.kenum was simply wrong as that can never happen.  The correct
check is for NULL u.kenum->names (or NULL u.kenum->values, either will
do).  Alternatively, debug_make_enum_type could be modified to make
u.kenum NULL when names or values are NULL, but that seems overkill
to fix a fuzzed object file segfault.

So my patch looks like this:

Subject: PR32829, SEGV on objdump function debug_type_samep

u.kenum is always non-NULL, see debug_make_enum_type.

	PR 32829
	* debug.c (debug_type_samep): Correct incomplete enum test.
	(debug_write_type): Remove dead code.

diff --git a/binutils/debug.c b/binutils/debug.c
index dcc8ccde4c5..465b18e7c0a 100644
--- a/binutils/debug.c
+++ b/binutils/debug.c
@@ -2554,9 +2554,6 @@ debug_write_type (struct debug_handle *info,
     case DEBUG_KIND_UNION_CLASS:
       return debug_write_class_type (info, fns, fhandle, type, tag);
     case DEBUG_KIND_ENUM:
-      if (type->u.kenum == NULL)
-	return (*fns->enum_type) (fhandle, tag, (const char **) NULL,
-				  (bfd_signed_vma *) NULL);
       return (*fns->enum_type) (fhandle, tag, type->u.kenum->names,
 				type->u.kenum->values);
     case DEBUG_KIND_POINTER:
@@ -3097,9 +3094,9 @@ debug_type_samep (struct debug_handle *info, struct debug_type_s *t1,
       break;
 
     case DEBUG_KIND_ENUM:
-      if (t1->u.kenum == NULL)
-	ret = t2->u.kenum == NULL;
-      else if (t2->u.kenum == NULL)
+      if (t1->u.kenum->names == NULL)
+	ret = t2->u.kenum->names == NULL;
+      else if (t2->u.kenum->names == NULL)
 	ret = false;
       else
 	{

> As an aside - aiui the PR also wants mentioning in the description (in a
> recognizable form), for the bugzilla entry to be automatically updated
> when the eventual fix was committed / pushed. I'm not sure mentioning it
> in the title and in this form is sufficient.

It works in the title if you write "PR 32829", but not as "PR32829".
I haven't tried "BZ #32829" but that did work at some stage given
https://sourceware.org/bugzilla/show_bug.cgi?id=18273#c8

-- 
Alan Modra