diff mbox series

Simplify note processing

Message ID mvmwoddq942.fsf@suse.de
State New
Headers show
Series Simplify note processing | expand

Commit Message

Andreas Schwab Oct. 10, 2019, 8:48 a.m. UTC 
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

Dmitry V. Levin Oct. 10, 2019, 10:19 a.m. UTC | #1 
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.
Andreas Schwab Oct. 10, 2019, 10:26 a.m. UTC | #2 
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.
Dmitry V. Levin Oct. 10, 2019, 10:38 a.m. UTC | #3 
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.
Florian Weimer Oct. 10, 2019, 12:01 p.m. UTC | #4 
* 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
diff mbox series

Patch

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;
 
 	    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)
 		  {