Re: [PATCH v4] readelf: Save and dump the original section header values

On 09.07.2026 09:11, H.J. Lu wrote:
> On Thu, Jul 9, 2026 at 2:55 PM Jan Beulich <[email protected]> wrote:
>>
>> On 09.07.2026 08:32, H.J. Lu wrote:
>>> On Thu, Jul 9, 2026 at 2:06 PM Jan Beulich <[email protected]> wrote:
>>>>
>>>> On 04.07.2026 13:06, H.J. Lu wrote:
>>>>> On Sat, Jul 4, 2026 at 11:01 AM H.J. Lu <[email protected]> wrote:
>>>>>> On Sat, Jul 4, 2026 at 8:39 AM H.J. Lu <[email protected]> wrote:
>>>>>>> On Sat, Jul 4, 2026 at 7:49 AM Alan Modra <[email protected]> wrote:
>>>>>>>>
>>>>>>>> On Fri, Jul 03, 2026 at 10:30:14PM +0800, H.J. Lu wrote:
>>>>>>>>> validate_section_info clears the garbage values in the section header
>>>>>>>>> to avoid crash later.  Save and dump the original section header values
>>>>>>>>> to make the garbage values in the section header visible when dumping
>>>>>>>>> section headers.
>>>>>>>>
>>>>>>>> I think this would be better done the other way around.  ie. have a
>>>>>>>> Elf_Internal_Shdr **sane_section_headers that is initialised to point
>>>>>>>> at entries in section_headers, with sane_section_headers[i] allocated
>>>>>>>> as necessary when needing to correct a bogus header.
>>>>>>>>
>>>>>>>
>>>>>>> I have thought about something similar and decided against it
>>>>>>> since filedata->section_headers is used in many places.   However,
>>>>>>> I can change filedata->orig_section_headers to on demand.
>>>>
>>>> I find this a plausible argument, so ...
>>>>
>>>>>> Changes in v2:
>>>>>>
>>>>>> 1.  filedata->orig_section_headers is changed to on demand.
>>>>>
>>>>> Changes in v3:
>>>>>
>>>>> 1.  Clear filedata->orig_section_headers first.
>>>>
>>>> ... the patch is okay unless you get an objection from Alan within another
>>>
>>> Will do.
>>>
>>>> day or two. Just one thing though: calloc() + memset(, 0,) is redundant.
>>>> Please drop those memset()s.
>>>
>>> It is cmalloc, not calloc:
>>>
>>>   filedata->orig_section_headers = (Elf_Internal_Shdr **)
>>>     cmalloc (num, sizeof (Elf_Internal_Shdr *));
>>>
>>> cmalloc doesn't clear memory.  memset is needed.
>>
>> Seeing that cmalloc() uses xmalloc() (not malloc()), why don't you then
>> simply use xcmalloc2()?
> 
> Did you mean xcalloc2?

Oh, sorry - yes, I did.

>> Further, again seeing that cmalloc() uses xmalloc() - the NULL checks
>> that you add are then solely to cover the multiplication overflow case.
>> That doesn't match the use of malloc() elsewhere in the patch.
>>
>> Taken together, why is it that calloc() isn't used here in the first
>> place (to match the use of malloc())? There's no truncation risk on the
>> first argument passed, and surely a sane implementation of calloc() has
>> to check for overflow of the multiplication when determining overall
> 
> I guess we don't know what calloc will be used.

Well, is it really on use to work around basic library flaws?

> Here is the v4 patch to use xcalloc2.

LGTM, thanks.

Jan