Simplify note processing

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

Commit Message

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

-- 
2.23.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Comments

Dmitry V. Levin Oct. 10, 2019, 10:19 a.m. | #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.


-- 
ldv
Andreas Schwab Oct. 10, 2019, 10:26 a.m. | #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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Dmitry V. Levin Oct. 10, 2019, 10:38 a.m. | #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.


-- 
ldv
Florian Weimer Oct. 10, 2019, 12:01 p.m. | #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

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