Commit Message
This removes dead code during note processing.
* elf/dl-load.c (open_verify): Remove dead code.
---
elf/dl-load.c | 17 +++--------------
1 file changed, 3 insertions(+), 14 deletions(-)
Comments
On Thu, Oct 10, 2019 at 10:48:29AM +0200, Andreas Schwab wrote:
> This removes dead code during note processing.
>
> * elf/dl-load.c (open_verify): Remove dead code.
> ---
> elf/dl-load.c | 17 +++--------------
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 24e2819345..1ed7a7bbd6 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1682,21 +1682,10 @@ open_verify (const char *name, int fd,
>
> /* Check .note.ABI-tag if present. */
> for (ph = phdr; ph < &phdr[ehdr->e_phnum]; ++ph)
> - if (ph->p_type == PT_NOTE && ph->p_filesz >= 32 && ph->p_align >= 4)
> + if (ph->p_type == PT_NOTE && ph->p_filesz >= 32
> + && (ph->p_align == 4 || ph->p_align == 8))
> {
> ElfW(Addr) size = ph->p_filesz;
> - /* NB: Some PT_NOTE segment may have alignment value of 0
> - or 1. gABI specifies that PT_NOTE segments should be
> - aligned to 4 bytes in 32-bit objects and to 8 bytes in
> - 64-bit objects. As a Linux extension, we also support
> - 4 byte alignment in 64-bit objects. If p_align is less
> - than 4, we treate alignment as 4 bytes since some note
> - segments have 0 or 1 byte alignment. */
> - ElfW(Addr) align = ph->p_align;
> - if (align < 4)
> - align = 4;
> - else if (align != 4 && align != 8)
> - continue;
This effectively removes support of ph->p_align < 4.
I think there should be an explanation e.g. in the commit message why
"Some PT_NOTE segment may have alignment value of 0 or 1" statement
is no longer true.
Besides that, there is a "free (abi_note_malloced)" right after the "if"
statement what looks suspicious: if free() is a no-op in this context,
why bother? If free() is not a no-op, then it's a chance of double free.
On Okt 10 2019, "Dmitry V. Levin" <ldv@altlinux.org> wrote:
> On Thu, Oct 10, 2019 at 10:48:29AM +0200, Andreas Schwab wrote:
>> This removes dead code during note processing.
>>
>> * elf/dl-load.c (open_verify): Remove dead code.
>> ---
>> elf/dl-load.c | 17 +++--------------
>> 1 file changed, 3 insertions(+), 14 deletions(-)
>>
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 24e2819345..1ed7a7bbd6 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -1682,21 +1682,10 @@ open_verify (const char *name, int fd,
>>
>> /* Check .note.ABI-tag if present. */
>> for (ph = phdr; ph < &phdr[ehdr->e_phnum]; ++ph)
>> - if (ph->p_type == PT_NOTE && ph->p_filesz >= 32 && ph->p_align >= 4)
>> + if (ph->p_type == PT_NOTE && ph->p_filesz >= 32
>> + && (ph->p_align == 4 || ph->p_align == 8))
>> {
>> ElfW(Addr) size = ph->p_filesz;
>> - /* NB: Some PT_NOTE segment may have alignment value of 0
>> - or 1. gABI specifies that PT_NOTE segments should be
>> - aligned to 4 bytes in 32-bit objects and to 8 bytes in
>> - 64-bit objects. As a Linux extension, we also support
>> - 4 byte alignment in 64-bit objects. If p_align is less
>> - than 4, we treate alignment as 4 bytes since some note
>> - segments have 0 or 1 byte alignment. */
>> - ElfW(Addr) align = ph->p_align;
>> - if (align < 4)
>> - align = 4;
>> - else if (align != 4 && align != 8)
>> - continue;
>
> This effectively removes support of ph->p_align < 4.
No. It was never supported. The condition ph->p_align >= 4 has been
there from the beginning.
Andreas.
On Thu, Oct 10, 2019 at 12:26:37PM +0200, Andreas Schwab wrote:
> On Okt 10 2019, "Dmitry V. Levin" <ldv@altlinux.org> wrote:
>
> > On Thu, Oct 10, 2019 at 10:48:29AM +0200, Andreas Schwab wrote:
> >> This removes dead code during note processing.
> >>
> >> * elf/dl-load.c (open_verify): Remove dead code.
> >> ---
> >> elf/dl-load.c | 17 +++--------------
> >> 1 file changed, 3 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/elf/dl-load.c b/elf/dl-load.c
> >> index 24e2819345..1ed7a7bbd6 100644
> >> --- a/elf/dl-load.c
> >> +++ b/elf/dl-load.c
> >> @@ -1682,21 +1682,10 @@ open_verify (const char *name, int fd,
> >>
> >> /* Check .note.ABI-tag if present. */
> >> for (ph = phdr; ph < &phdr[ehdr->e_phnum]; ++ph)
> >> - if (ph->p_type == PT_NOTE && ph->p_filesz >= 32 && ph->p_align >= 4)
> >> + if (ph->p_type == PT_NOTE && ph->p_filesz >= 32
> >> + && (ph->p_align == 4 || ph->p_align == 8))
> >> {
> >> ElfW(Addr) size = ph->p_filesz;
> >> - /* NB: Some PT_NOTE segment may have alignment value of 0
> >> - or 1. gABI specifies that PT_NOTE segments should be
> >> - aligned to 4 bytes in 32-bit objects and to 8 bytes in
> >> - 64-bit objects. As a Linux extension, we also support
> >> - 4 byte alignment in 64-bit objects. If p_align is less
> >> - than 4, we treate alignment as 4 bytes since some note
> >> - segments have 0 or 1 byte alignment. */
> >> - ElfW(Addr) align = ph->p_align;
> >> - if (align < 4)
> >> - align = 4;
> >> - else if (align != 4 && align != 8)
> >> - continue;
> >
> > This effectively removes support of ph->p_align < 4.
>
> No. It was never supported. The condition ph->p_align >= 4 has been
> there from the beginning.
Indeed, and the "free (abi_note_malloced)" is actually outside the loop,
I must've completely misread the code, sorry for the noise.
The change is fine then.
* Dmitry V. Levin:
> On Thu, Oct 10, 2019 at 12:26:37PM +0200, Andreas Schwab wrote:
>> On Okt 10 2019, "Dmitry V. Levin" <ldv@altlinux.org> wrote:
>>
>> > On Thu, Oct 10, 2019 at 10:48:29AM +0200, Andreas Schwab wrote:
>> >> This removes dead code during note processing.
>> >>
>> >> * elf/dl-load.c (open_verify): Remove dead code.
>> >> ---
>> >> elf/dl-load.c | 17 +++--------------
>> >> 1 file changed, 3 insertions(+), 14 deletions(-)
>> >>
>> >> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> >> index 24e2819345..1ed7a7bbd6 100644
>> >> --- a/elf/dl-load.c
>> >> +++ b/elf/dl-load.c
>> >> @@ -1682,21 +1682,10 @@ open_verify (const char *name, int fd,
>> >>
>> >> /* Check .note.ABI-tag if present. */
>> >> for (ph = phdr; ph < &phdr[ehdr->e_phnum]; ++ph)
>> >> - if (ph->p_type == PT_NOTE && ph->p_filesz >= 32 && ph->p_align >= 4)
>> >> + if (ph->p_type == PT_NOTE && ph->p_filesz >= 32
>> >> + && (ph->p_align == 4 || ph->p_align == 8))
>> >> {
>> >> ElfW(Addr) size = ph->p_filesz;
>> >> - /* NB: Some PT_NOTE segment may have alignment value of 0
>> >> - or 1. gABI specifies that PT_NOTE segments should be
>> >> - aligned to 4 bytes in 32-bit objects and to 8 bytes in
>> >> - 64-bit objects. As a Linux extension, we also support
>> >> - 4 byte alignment in 64-bit objects. If p_align is less
>> >> - than 4, we treate alignment as 4 bytes since some note
>> >> - segments have 0 or 1 byte alignment. */
>> >> - ElfW(Addr) align = ph->p_align;
>> >> - if (align < 4)
>> >> - align = 4;
>> >> - else if (align != 4 && align != 8)
>> >> - continue;
>> >
>> > This effectively removes support of ph->p_align < 4.
>>
>> No. It was never supported. The condition ph->p_align >= 4 has been
>> there from the beginning.
>
> Indeed, and the "free (abi_note_malloced)" is actually outside the loop,
> I must've completely misread the code, sorry for the noise.
>
> The change is fine then.
I agree. I believe alignment of 0 and 1 is only encountered in Linux
core files, which does not matter to this note parser.
Thanks,
Florian
@@ -1682,21 +1682,10 @@ open_verify (const char *name, int fd,
/* Check .note.ABI-tag if present. */
for (ph = phdr; ph < &phdr[ehdr->e_phnum]; ++ph)
- if (ph->p_type == PT_NOTE && ph->p_filesz >= 32 && ph->p_align >= 4)
+ if (ph->p_type == PT_NOTE && ph->p_filesz >= 32
+ && (ph->p_align == 4 || ph->p_align == 8))
{
ElfW(Addr) size = ph->p_filesz;
- /* NB: Some PT_NOTE segment may have alignment value of 0
- or 1. gABI specifies that PT_NOTE segments should be
- aligned to 4 bytes in 32-bit objects and to 8 bytes in
- 64-bit objects. As a Linux extension, we also support
- 4 byte alignment in 64-bit objects. If p_align is less
- than 4, we treate alignment as 4 bytes since some note
- segments have 0 or 1 byte alignment. */
- ElfW(Addr) align = ph->p_align;
- if (align < 4)
- align = 4;
- else if (align != 4 && align != 8)
- continue;
if (ph->p_offset + size <= (size_t) fbp->len)
abi_note = (void *) (fbp->buf + ph->p_offset);
@@ -1727,7 +1716,7 @@ open_verify (const char *name, int fd,
{
ElfW(Addr) note_size
= ELF_NOTE_NEXT_OFFSET (abi_note[0], abi_note[1],
- align);
+ ph->p_align);
if (size - 32 < note_size)
{