Re: [PATCH V4 1/4] Add dso__is_pie call to identify ELF PIE

[Leo Yan <leo.yan@arm.com>]

On 8/28/2024 4:48 PM, Steve Clevenger wrote:
> 
> On 8/28/2024 1:23 AM, Leo Yan wrote:
>> On 8/28/2024 6:09 AM, Steve Clevenger wrote:
>>>
>>> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for
>>> the DF_1_PIE flag. This identifies position executable code.
>>>
>>> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
>>> ---
>>>  tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++
>>>  tools/perf/util/symbol.h     |  1 +
>>>  2 files changed, 61 insertions(+)
>>>
>>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>>> index e398abfd13a0..0e49c1345a67 100644
>>> --- a/tools/perf/util/symbol-elf.c
>>> +++ b/tools/perf/util/symbol-elf.c
>>> @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf,
>>>         return err;
>>>  }
>>>
>>> +/*
>>> + * Check dynamic section DT_FLAGS_1 for a Position Independent
>>> + * Executable (PIE).
>>> + */
>>> +bool dso__is_pie(struct dso *dso)
>>> +{
>>> +       Elf *elf = NULL;
>>> +       Elf_Scn *scn = NULL;
>>> +       GElf_Ehdr ehdr;
>>> +       GElf_Shdr shdr;
>>> +       bool is_pie = false;
>>> +       char dso_path[PATH_MAX];
>>> +       int fd = -1;
>>> +
>>> +       if (!dso || (elf_version(EV_CURRENT) == EV_NONE))
>>> +               goto exit;      // false
>>> +
>>> +       dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
>>> +
>>> +       fd = open(dso_path, O_RDONLY);
>>> +
>>> +       if (fd < 0) {
>>> +               pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
>>> +               goto exit;      // false
>>> +       }
>>> +
>>> +       elf = elf_begin(fd, ELF_C_READ, NULL);
>>> +       gelf_getehdr(elf, &ehdr);
>>> +
>>> +       if (ehdr.e_type == ET_DYN) {
>>> +               Elf_Data *data;
>>> +               GElf_Dyn *entry;
>>> +               int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
>>> +
>>> +               scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);
>>> +               if (!scn)
>>> +                       goto exit;      // false
>>
>> It should goto a new tag (e.g. exit_failed) so can invoke elf_end(elf) and
>> close(fd).
>>
>>> +
>>> +               data = (Elf_Data *) elf_getdata(scn, NULL);
>>> +               if (!data || !data->d_buf)
>>> +                       goto exit;      // false
>>
>> Ditto.
>>
>>> +
>>> +               // check DT_FLAGS_1
>>> +               for (int i = 0; i < n_entries; i++) {
>>> +                       entry = ((GElf_Dyn *) data->d_buf) + i;
>>> +                       if (entry->d_tag == DT_FLAGS_1) {
>>> +                               if ((entry->d_un.d_val & DF_1_PIE) != 0) {
>>> +                                       is_pie = true;
>>> +                                       break;
>>> +                               }
>>> +                       }
>>> +               } // end for
>>> +       }
>>> +
>>
>> Put 'exit_failed' tag at here.
>>
>> With the changes:
>>
>> Reviewed-by: Leo Yan <leo.yan@arm.com>
>>
>>> +       elf_end(elf);
>>> +       close(fd);
>>> +exit:
>>> +       return is_pie;
>>> +}
>>> +
> 
> Hi Leo,
> 
> It's been a long time since I've seen goto's used as a rule rather than
> an exception. I see it introduced the problem you've mentioned.

I am not sure if I understand this. In the Linux code, it is quite common to
use goto to handle failure cases (for releasing resources).

> Is it ok to simply update the cover letter (patch 0), and this one (patch 1)
> as V5? If I don't hear from you, I'll resubmit all.

I assume you also need to address patch 03? It is good to send the all patches
in one go, this would be easier for maintainers to pick up the whole series
(e.g. using b4).

Thanks,
Leo