This is the mail archive of the binutils@sources.redhat.com mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] readelf robustification (part N)


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]