diff mbox

[2/2] elf: Use nocancel pread64() instead of lseek()+read()

Message ID MWHPR2101MB073287753F9E8D211AF49DD3E8DA0@MWHPR2101MB0732.namprd21.prod.outlook.com
State Committed
Headers

Commit Message

Leandro Pereira Aug. 5, 2019, 9:56 p.m. UTC 
  Transforms this, when linking in a shared object:

  openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
  read(3, "\177ELF\2\1\1\3"..., 832) = 832
  lseek(3, 792, SEEK_SET)           = 792
  read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
  fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
  lseek(3, 792, SEEK_SET)           = 792
  read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
  lseek(3, 864, SEEK_SET)           = 864
  read(3, "\4\0\0\0\20\0\0\0"..., 32) = 32

Into this:

  openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
  read(3, "\177ELF\2\1\1\3"..., 832) = 832
  pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
  fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
  pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
  pread(3, "\4\0\0\0\20\0\0\0"..., 32, 864) = 32

2019-08-05  Leandro Pereira  <leandro.pereira@microsoft.com>

	* elf/dl-load.c: Use __pread64_nocancel() instead of __lseek()+
	__read_nocancel().
	* sysdeps/x86/dl-prop.h: Likewise.

---
 elf/dl-load.c         | 9 +++------
 sysdeps/x86/dl-prop.h | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

Comments

Carlos O'Donell Oct. 3, 2019, 1:59 p.m. UTC | #1 
On 8/5/19 5:56 PM, Leandro Pereira wrote:
> Transforms this, when linking in a shared object:
> 
>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>   lseek(3, 792, SEEK_SET)           = 792
>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>   lseek(3, 792, SEEK_SET)           = 792
>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>   lseek(3, 864, SEEK_SET)           = 864
>   read(3, "\4\0\0\0\20\0\0\0"..., 32) = 32
> 
> Into this:
> 
>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>   pread(3, "\4\0\0\0\20\0\0\0"..., 32, 864) = 32
> 
> 2019-08-05  Leandro Pereira  <leandro.pereira@microsoft.com>
> 
> 	* elf/dl-load.c: Use __pread64_nocancel() instead of __lseek()+
> 	__read_nocancel().
> 	* sysdeps/x86/dl-prop.h: Likewise.

OK for master. I'll push after testing with bmg. No regressions on x86_64.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/dl-load.c         | 9 +++------
>  sysdeps/x86/dl-prop.h | 3 +--
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 5abeb867f1..462c425a13 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1005,8 +1005,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>    else
>      {

OK. I reviewed that all uses of fd did not depend on the SEEK_SET being set to
a given position.

>        phdr = alloca (maplength);
> -      __lseek (fd, header->e_phoff, SEEK_SET);
> -      if ((size_t) __read_nocancel (fd, (void *) phdr, maplength) != maplength)
> +      if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength, header->e_phoff) != maplength)

This line is too long, but I'll wrap it before commit, likewise for 2 other lines.

>  	{
>  	  errstring = N_("cannot read file data");
>  	  goto call_lose_errno;
> @@ -1659,8 +1658,7 @@ open_verify (const char *name, int fd,
>        else
>  	{
>  	  phdr = alloca (maplength);
> -	  __lseek (fd, ehdr->e_phoff, SEEK_SET);
> -	  if ((size_t) __read_nocancel (fd, (void *) phdr, maplength)
> +	  if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength, ehdr->e_phoff)

OK. Wrapped.

>  	      != maplength)
>  	    {
>  	    read_error:
> @@ -1710,8 +1708,7 @@ open_verify (const char *name, int fd,
>  
>  		    abi_note = abi_note_malloced;
>  		  }
> -		__lseek (fd, ph->p_offset, SEEK_SET);
> -		if (__read_nocancel (fd, (void *) abi_note, size) != size)
> +		if (__pread64_nocancel (fd, (void *) abi_note, size, ph->p_offset) != size)

OK. Wrapped.

>  		  {
>  		    free (abi_note_malloced);
>  		    goto read_error;
> diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
> index 1b335ccbb3..080d66a971 100644
> --- a/sysdeps/x86/dl-prop.h
> +++ b/sysdeps/x86/dl-prop.h
> @@ -167,8 +167,7 @@ _dl_process_pt_note (struct link_map *l, const ElfW(Phdr) *ph,
>  	  note_malloced = malloc (size);
>  	  note = note_malloced;
>  	}
> -      __lseek (fd, ph->p_offset, SEEK_SET);
> -      if (__read_nocancel (fd, (void *) note, size) != size)
> +      if (__pread64_nocancel (fd, (void *) note, size, ph->p_offset) != size)

OK.

>  	{
>  	  if (note_malloced)
>  	    free (note_malloced);
>
Yann Droneaud Oct. 3, 2019, 3:31 p.m. UTC | #2 
Hi,

Le lundi 05 août 2019 à 21:56 +0000, Leandro Pereira a écrit :
> Transforms this, when linking in a shared object:
> 
>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>   lseek(3, 792, SEEK_SET)           = 792
>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>   lseek(3, 792, SEEK_SET)           = 792
>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>   lseek(3, 864, SEEK_SET)           = 864
>   read(3, "\4\0\0\0\20\0\0\0"..., 32) = 32
> 
> Into this:
> 
>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>   pread(3, "\4\0\0\0\20\0\0\0"..., 32, 864) = 32
> 

I confirm the behavior. And the usefulness of the patch.

Anyway I'm quite surprised ld.so is reading twice 68 bytes at the same
792 offset. Moreover, the first read already brought 28 bytes out of
68.

Could these read() be replaced by a call to mmap(), then using mremap()
to brought the shared object in memory after initial inspection ?

Regards.
Carlos O'Donell Oct. 3, 2019, 5:10 p.m. UTC | #3 
On 10/3/19 11:31 AM, Yann Droneaud wrote:
> Hi,
> 
> Le lundi 05 août 2019 à 21:56 +0000, Leandro Pereira a écrit :
>> Transforms this, when linking in a shared object:
>>
>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>   lseek(3, 792, SEEK_SET)           = 792
>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>   lseek(3, 792, SEEK_SET)           = 792
>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>   lseek(3, 864, SEEK_SET)           = 864
>>   read(3, "\4\0\0\0\20\0\0\0"..., 32) = 32
>>
>> Into this:
>>
>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>   pread(3, "\4\0\0\0\20\0\0\0"..., 32, 864) = 32
>>
> 
> I confirm the behavior. And the usefulness of the patch.
> 
> Anyway I'm quite surprised ld.so is reading twice 68 bytes at the same
> 792 offset. Moreover, the first read already brought 28 bytes out of
> 68.
> 
> Could these read() be replaced by a call to mmap(), then using mremap()
> to brought the shared object in memory after initial inspection ?

It might be possible. It's an optimization that would probably require
some refactoring and review of the current logic in the DSO loading.

Happy to review patches :-)
Andreas Schwab Oct. 3, 2019, 5:59 p.m. UTC | #4 
On Okt 03 2019, Yann Droneaud <ydroneaud@opteya.com> wrote:

> Anyway I'm quite surprised ld.so is reading twice 68 bytes at the same
> 792 offset. Moreover, the first read already brought 28 bytes out of
> 68.

That only happens if you have a note segment that does not fit in the
initial file buffer.  That is rather unusual.

Andreas.
Yann Droneaud Oct. 3, 2019, 7:04 p.m. UTC | #5 
Hi,

Le jeudi 03 octobre 2019 à 19:59 +0200, Andreas Schwab a écrit :
> On Okt 03 2019, Yann Droneaud <ydroneaud@opteya.com> wrote:
> 
> > Anyway I'm quite surprised ld.so is reading twice 68 bytes at the
> > same
> > 792 offset. Moreover, the first read already brought 28 bytes out
> > of
> > 68.
> 
> That only happens if you have a note segment that does not fit in the
> initial file buffer.  That is rather unusual.
> 

Not sure about it being unusual: on Fedora 30, x86_64, I see this
behavior with /bin/false /bin/true /bin/test /bin/uname 

openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
read(3, "\177ELF\2\1\1\3"..., 832)      = 832
lseek(3, 792, SEEK_SET)                 = 792
read(3, "\4\0\0\0\24\0\0\0"..., 68)     = 68
fstat(3, {st_mode=S_IFREG|0755, st_size=6697832, ...}) = 0
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f8bc04b5000
lseek(3, 792, SEEK_SET)                 = 792
read(3, "\4\0\0\0\24\0\0\0"..., 68)     = 68
lseek(3, 864, SEEK_SET)                 = 864
read(3, "\4\0\0\0\20\0\0\0"..., 32)     = 32


$ file /bin/false /bin/true /bin/test /bin/uname 
/bin/false: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV),
dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for
GNU/Linux 3.2.0,
BuildID[sha1]=790d23abaaa68bb496873f5afc647dd6be30f94c, stripped, too
many notes (256)
/bin/true:  ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV),
dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for
GNU/Linux 3.2.0,
BuildID[sha1]=8e348e43f020cfa72b8abed7e69b54558165fb2e, stripped, too
many notes (256)
/bin/test:  ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV),
dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for
GNU/Linux 3.2.0,
BuildID[sha1]=5dee19457e437541fe30f58d3d4add118567e806, stripped, too
many notes (256)
/bin/uname: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV),
dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for
GNU/Linux 3.2.0,
BuildID[sha1]=50b398bf94ad5a081eb4178d976d4f290977f99a, stripped, too
many notes (256)

Too many notes ?

Regards.
Andreas Schwab Oct. 3, 2019, 7:16 p.m. UTC | #6 
On Okt 03 2019, Yann Droneaud <ydroneaud@opteya.com> wrote:

> Not sure about it being unusual: on Fedora 30, x86_64, I see this
> behavior with /bin/false /bin/true /bin/test /bin/uname 

I'd say that's a bug in Fedora, then.

Andreas.
Carlos O'Donell Oct. 18, 2019, 8:02 p.m. UTC | #7 
On 10/3/19 9:59 AM, Carlos O'Donell wrote:
> On 8/5/19 5:56 PM, Leandro Pereira wrote:
>> Transforms this, when linking in a shared object:
>>
>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>   lseek(3, 792, SEEK_SET)           = 792
>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>   lseek(3, 792, SEEK_SET)           = 792
>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>   lseek(3, 864, SEEK_SET)           = 864
>>   read(3, "\4\0\0\0\20\0\0\0"..., 32) = 32
>>
>> Into this:
>>
>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>   pread(3, "\4\0\0\0\20\0\0\0"..., 32, 864) = 32
>>
>> 2019-08-05  Leandro Pereira  <leandro.pereira@microsoft.com>
>>
>> 	* elf/dl-load.c: Use __pread64_nocancel() instead of __lseek()+
>> 	__read_nocancel().
>> 	* sysdeps/x86/dl-prop.h: Likewise.
> 
> OK for master. I'll push after testing with bmg. No regressions on x86_64.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Pushed.
Lukasz Majewski Oct. 20, 2019, 9:37 p.m. UTC | #8 
On Fri, 18 Oct 2019 16:02:22 -0400
Carlos O'Donell <carlos@redhat.com> wrote:

> On 10/3/19 9:59 AM, Carlos O'Donell wrote:
> > On 8/5/19 5:56 PM, Leandro Pereira wrote:  
> >> Transforms this, when linking in a shared object:
> >>
> >>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
> >>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
> >>   lseek(3, 792, SEEK_SET)           = 792
> >>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
> >>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
> >>   lseek(3, 792, SEEK_SET)           = 792
> >>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
> >>   lseek(3, 864, SEEK_SET)           = 864
> >>   read(3, "\4\0\0\0\20\0\0\0"..., 32) = 32
> >>
> >> Into this:
> >>
> >>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
> >>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
> >>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
> >>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
> >>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
> >>   pread(3, "\4\0\0\0\20\0\0\0"..., 32, 864) = 32
> >>
> >> 2019-08-05  Leandro Pereira  <leandro.pereira@microsoft.com>
> >>
> >> 	* elf/dl-load.c: Use __pread64_nocancel() instead of
> >> __lseek()+ __read_nocancel().
> >> 	* sysdeps/x86/dl-prop.h: Likewise.  
> > 
> > OK for master. I'll push after testing with bmg. No regressions on
> > x86_64.
> > 
> > Reviewed-by: Carlos O'Donell <carlos@redhat.com>  
> 
> Pushed.
> 

Unfortunately, I've found a regression on HURD when using
build-many-glibcs.py (for testing some other code):

../glibc-many-build/src/glibc/libio/../sysdeps/posix/libc_fatal.c:161:
multiple definition of `__libc_fatal';
../glibc-many-build/build/glibcs/i686-gnu/glibc/elf/dl-allobjs.os:/work/lukma/glibc/glibc-many-build/src/glibc/elf/dl-minimal.c\
:188: first defined here

To reproduce:
../src/scripts/build-many-glibcs.py
<path>/glibc/glibc-many-build glibcs i686-gnu

emacs logs/glibcs/i686-gnu/004-glibcs-i686-gnu-build-log.txt




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Carlos O'Donell Oct. 21, 2019, 1:24 p.m. UTC | #9 
On 10/20/19 5:37 PM, Lukasz Majewski wrote:
> On Fri, 18 Oct 2019 16:02:22 -0400
> Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> On 10/3/19 9:59 AM, Carlos O'Donell wrote:
>>> On 8/5/19 5:56 PM, Leandro Pereira wrote:  
>>>> Transforms this, when linking in a shared object:
>>>>
>>>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>>>   lseek(3, 792, SEEK_SET)           = 792
>>>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>>>   lseek(3, 792, SEEK_SET)           = 792
>>>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>>>   lseek(3, 864, SEEK_SET)           = 864
>>>>   read(3, "\4\0\0\0\20\0\0\0"..., 32) = 32
>>>>
>>>> Into this:
>>>>
>>>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>>>   pread(3, "\4\0\0\0\20\0\0\0"..., 32, 864) = 32
>>>>
>>>> 2019-08-05  Leandro Pereira  <leandro.pereira@microsoft.com>
>>>>
>>>> 	* elf/dl-load.c: Use __pread64_nocancel() instead of
>>>> __lseek()+ __read_nocancel().
>>>> 	* sysdeps/x86/dl-prop.h: Likewise.  
>>>
>>> OK for master. I'll push after testing with bmg. No regressions on
>>> x86_64.
>>>
>>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>  
>>
>> Pushed.
>>
> 
> Unfortunately, I've found a regression on HURD when using
> build-many-glibcs.py (for testing some other code):
> 
> ../glibc-many-build/src/glibc/libio/../sysdeps/posix/libc_fatal.c:161:
> multiple definition of `__libc_fatal';
> ../glibc-many-build/build/glibcs/i686-gnu/glibc/elf/dl-allobjs.os:/work/lukma/glibc/glibc-many-build/src/glibc/elf/dl-minimal.c\
> :188: first defined here
> 
> To reproduce:
> ../src/scripts/build-many-glibcs.py
> <path>/glibc/glibc-many-build glibcs i686-gnu
> 
> emacs logs/glibcs/i686-gnu/004-glibcs-i686-gnu-build-log.txt

Thanks. I didn't see this. Let me start another build.
Florian Weimer Oct. 24, 2019, 12:08 p.m. UTC | #10 
* Carlos O'Donell:

> On 10/20/19 5:37 PM, Lukasz Majewski wrote:
>> On Fri, 18 Oct 2019 16:02:22 -0400
>> Carlos O'Donell <carlos@redhat.com> wrote:
>> 
>>> On 10/3/19 9:59 AM, Carlos O'Donell wrote:
>>>> On 8/5/19 5:56 PM, Leandro Pereira wrote:  
>>>>> Transforms this, when linking in a shared object:
>>>>>
>>>>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>>>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>>>>   lseek(3, 792, SEEK_SET)           = 792
>>>>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>>>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>>>>   lseek(3, 792, SEEK_SET)           = 792
>>>>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>>>>   lseek(3, 864, SEEK_SET)           = 864
>>>>>   read(3, "\4\0\0\0\20\0\0\0"..., 32) = 32
>>>>>
>>>>> Into this:
>>>>>
>>>>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>>>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>>>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>>>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>>>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>>>>   pread(3, "\4\0\0\0\20\0\0\0"..., 32, 864) = 32
>>>>>
>>>>> 2019-08-05  Leandro Pereira  <leandro.pereira@microsoft.com>
>>>>>
>>>>> 	* elf/dl-load.c: Use __pread64_nocancel() instead of
>>>>> __lseek()+ __read_nocancel().
>>>>> 	* sysdeps/x86/dl-prop.h: Likewise.  
>>>>
>>>> OK for master. I'll push after testing with bmg. No regressions on
>>>> x86_64.
>>>>
>>>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>  
>>>
>>> Pushed.
>>>
>> 
>> Unfortunately, I've found a regression on HURD when using
>> build-many-glibcs.py (for testing some other code):
>> 
>> ../glibc-many-build/src/glibc/libio/../sysdeps/posix/libc_fatal.c:161:
>> multiple definition of `__libc_fatal';
>> ../glibc-many-build/build/glibcs/i686-gnu/glibc/elf/dl-allobjs.os:/work/lukma/glibc/glibc-many-build/src/glibc/elf/dl-minimal.c\
>> :188: first defined here
>> 
>> To reproduce:
>> ../src/scripts/build-many-glibcs.py
>> <path>/glibc/glibc-many-build glibcs i686-gnu
>> 
>> emacs logs/glibcs/i686-gnu/004-glibcs-i686-gnu-build-log.txt
>
> Thanks. I didn't see this. Let me start another build.

I think I know what's going on and I'm looking to a fix.

We shouldn't let such build regressions linger for so long.
Carlos O'Donell Oct. 24, 2019, 1:32 p.m. UTC | #11 
On 10/24/19 8:08 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 10/20/19 5:37 PM, Lukasz Majewski wrote:
>>> On Fri, 18 Oct 2019 16:02:22 -0400
>>> Carlos O'Donell <carlos@redhat.com> wrote:
>>>
>>>> On 10/3/19 9:59 AM, Carlos O'Donell wrote:
>>>>> On 8/5/19 5:56 PM, Leandro Pereira wrote:  
>>>>>> Transforms this, when linking in a shared object:
>>>>>>
>>>>>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>>>>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>>>>>   lseek(3, 792, SEEK_SET)           = 792
>>>>>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>>>>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>>>>>   lseek(3, 792, SEEK_SET)           = 792
>>>>>>   read(3, "\4\0\0\0\24\0\0\0"..., 68) = 68
>>>>>>   lseek(3, 864, SEEK_SET)           = 864
>>>>>>   read(3, "\4\0\0\0\20\0\0\0"..., 32) = 32
>>>>>>
>>>>>> Into this:
>>>>>>
>>>>>>   openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
>>>>>>   read(3, "\177ELF\2\1\1\3"..., 832) = 832
>>>>>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>>>>>   fstat(3, {st_mode=S_IFREG|0755, st_size=6699224, ...}) = 0
>>>>>>   pread(3, "\4\0\0\0\24\0\0\0"..., 68, 792) = 68
>>>>>>   pread(3, "\4\0\0\0\20\0\0\0"..., 32, 864) = 32
>>>>>>
>>>>>> 2019-08-05  Leandro Pereira  <leandro.pereira@microsoft.com>
>>>>>>
>>>>>> 	* elf/dl-load.c: Use __pread64_nocancel() instead of
>>>>>> __lseek()+ __read_nocancel().
>>>>>> 	* sysdeps/x86/dl-prop.h: Likewise.  
>>>>>
>>>>> OK for master. I'll push after testing with bmg. No regressions on
>>>>> x86_64.
>>>>>
>>>>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>  
>>>>
>>>> Pushed.
>>>>
>>>
>>> Unfortunately, I've found a regression on HURD when using
>>> build-many-glibcs.py (for testing some other code):
>>>
>>> ../glibc-many-build/src/glibc/libio/../sysdeps/posix/libc_fatal.c:161:
>>> multiple definition of `__libc_fatal';
>>> ../glibc-many-build/build/glibcs/i686-gnu/glibc/elf/dl-allobjs.os:/work/lukma/glibc/glibc-many-build/src/glibc/elf/dl-minimal.c\
>>> :188: first defined here
>>>
>>> To reproduce:
>>> ../src/scripts/build-many-glibcs.py
>>> <path>/glibc/glibc-many-build glibcs i686-gnu
>>>
>>> emacs logs/glibcs/i686-gnu/004-glibcs-i686-gnu-build-log.txt
>>
>> Thanks. I didn't see this. Let me start another build.
> 
> I think I know what's going on and I'm looking to a fix.
> 
> We shouldn't let such build regressions linger for so long.
 
Sorry, you are right 4 days is too long. It speaks again for pre-commit
CI since even straight forward things can have consequences like this.
I ran b-m-g and didn't see this, but perhaps I made a mistake. At least
twice I've botched the glibc branch checkout and tested b-m-g against
upstream instead of upstream+patch.

You and I have spoken about this off list and you have a patch, so I'm
going to leave this with you to fix while I go review the CodeSourcery/Mentor
patches.
Florian Weimer Oct. 24, 2019, 1:46 p.m. UTC | #12 
* Carlos O'Donell:

>>>> emacs logs/glibcs/i686-gnu/004-glibcs-i686-gnu-build-log.txt
>>>
>>> Thanks. I didn't see this. Let me start another build.
>> 
>> I think I know what's going on and I'm looking to a fix.
>> 
>> We shouldn't let such build regressions linger for so long.
>  
> Sorry, you are right 4 days is too long. It speaks again for pre-commit
> CI since even straight forward things can have consequences like this.
> I ran b-m-g and didn't see this, but perhaps I made a mistake. At least
> twice I've botched the glibc branch checkout and tested b-m-g against
> upstream instead of upstream+patch.
>
> You and I have spoken about this off list and you have a patch, so I'm
> going to leave this with you to fix while I go review the CodeSourcery/Mentor
> patches.

I posted a patch:

  <https://sourceware.org/ml/libc-alpha/2019-10/msg00706.html>

Thanks,
Florian
diff mbox

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 5abeb867f1..462c425a13 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1005,8 +1005,7 @@  _dl_map_object_from_fd (const char *name, const char *origname, int fd,
   else
     {
       phdr = alloca (maplength);
-      __lseek (fd, header->e_phoff, SEEK_SET);
-      if ((size_t) __read_nocancel (fd, (void *) phdr, maplength) != maplength)
+      if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength, header->e_phoff) != maplength)
 	{
 	  errstring = N_("cannot read file data");
 	  goto call_lose_errno;
@@ -1659,8 +1658,7 @@  open_verify (const char *name, int fd,
       else
 	{
 	  phdr = alloca (maplength);
-	  __lseek (fd, ehdr->e_phoff, SEEK_SET);
-	  if ((size_t) __read_nocancel (fd, (void *) phdr, maplength)
+	  if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength, ehdr->e_phoff)
 	      != maplength)
 	    {
 	    read_error:
@@ -1710,8 +1708,7 @@  open_verify (const char *name, int fd,
 
 		    abi_note = abi_note_malloced;
 		  }
-		__lseek (fd, ph->p_offset, SEEK_SET);
-		if (__read_nocancel (fd, (void *) abi_note, size) != size)
+		if (__pread64_nocancel (fd, (void *) abi_note, size, ph->p_offset) != size)
 		  {
 		    free (abi_note_malloced);
 		    goto read_error;
diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
index 1b335ccbb3..080d66a971 100644
--- a/sysdeps/x86/dl-prop.h
+++ b/sysdeps/x86/dl-prop.h
@@ -167,8 +167,7 @@  _dl_process_pt_note (struct link_map *l, const ElfW(Phdr) *ph,
 	  note_malloced = malloc (size);
 	  note = note_malloced;
 	}
-      __lseek (fd, ph->p_offset, SEEK_SET);
-      if (__read_nocancel (fd, (void *) note, size) != size)
+      if (__pread64_nocancel (fd, (void *) note, size, ph->p_offset) != size)
 	{
 	  if (note_malloced)
 	    free (note_malloced);