[PATCH] readelf robustification (part N)

[Nick Clifton]

Hi Jakub,

> 2005-06-14  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* readelf.c (CHECK_ENTSIZE_VALUES, CHECK_ENTSIZE): Define.
> 	(process_section_headers): Use it.
> 	(process_relocs): Don't crash if symsec is not SHT_SYMTAB
> 	or SHT_DYNSYM.
> 	(process_version_sections): Use 2 instead of sh_entsize.

This is mostly OK, but:

> +#define CHECK_ENTSIZE_VALUES(section, i, size32, size64) \
> +  do {									    \
> +    size_t expected_entsize						    \
> +      = is_32bit_elf ? size32 : size64;					    \
> +    if (section->sh_entsize != expected_entsize)			    \
> +      error (_("Section %d has invalid sh_entsize %lx (expected %lx)\n"),   \
> +	     i, (unsigned long int) section->sh_entsize,		    \
> +	     (unsigned long int) expected_entsize);			    \
> +    section->sh_entsize = expected_entsize;				    \
> +  } while (0)

Formatting.  Just because this is a macro there is no reason to abandon 
the GNU Coding standard.  Please treat the macro like ordinary code and 
break out the curly braces to their own lines.


> +      else if (section->sh_type == SHT_GROUP)
> +	CHECK_ENTSIZE_VALUES (section, i, 4, 4);

I am slightly worried about the appearance of the "4" here.  I do not 
like magic constants in code, although I see that "4" has been used in 
this context elsewhere in the readelf sources.  I would be much happier 
if we had a macro value here instead. eg:

   #define GROUP_ENTRY_SIZE  4

Then this could be used elsewhere in the group processing code as well.


> -	    total = section->sh_size / section->sh_entsize;
> +	    total = section->sh_size / 2;

I am worried by this "2" as well.  If you are concerned that sh_entsize 
might be invalid here, shouldn't you be checking it ?  Also I see that 
the code further on uses "sizeof (short)" instead of "2", but this seems 
unsafe to me, since there is no guarantee that the size of a short on 
the system running readelf is 2.  (It might be 1 on a 16-bit host...)

I think that we need another defined constant here (eg 
VERSYM_ENTRY_SIZE) and a check to make size that "sizeof (*data) >= 
VERSYM_ENTRY_SIZE".

Cheers
   Nick