Patchwork weak symbols need to be "defined" weak but "declared" strong

login
register
mail settings
Submitter Vineet Gupta
Date April 16, 2014, 10:36 a.m.
Message ID <1397644605-13526-1-git-send-email-vgupta@synopsys.com>
Download mbox | patch
Permalink /patch/339545/
State Accepted
Commit 00f425b39b9258a78df47cebdd0d2c779753392f
Headers show

Comments

Vineet Gupta - April 16, 2014, 10:36 a.m.
Patch "LT.old: Make __errno_location/__h_errno_location thread safe"
uncovered yet another bug with static linking and errno (hopefully this
is last of them all).

Currently, __errno_location is declared weak but is defined strong.
While this provides with the desired weak semantics in dso, it
is subtly broken in static links.

Quoting Joern Rennecke (ARC gcc expert):

| I think the issue is that you declare the function as weak in the
| header file.  That is a rare instance where you want the reference
| use declaration that differs a bit from the definition.
| If the reference uses a weakly declared function, that creates a
| weakref, i.e. the linker won't bother to look for this symbol at
|  all - if it gets linked in for some other reason, fine,
| otherwise, it stays zero.

So the solution to declare strong, define weak.

Supporting data
-----------------
orig code: ARM mmap wrapper (LT.old build + my prev patch for errno)

_mmap:
    @ args = 8, pretend = 0, frame = 0
    @ frame_needed = 0, uses_anonymous_args = 0
    stmfd    sp!, {r4, r5, r7, lr}
    ldr    r5, [sp, #20]
    movs    ip, r5, asl #20
    beq    .L2
    bl    __errno_location(PLT)
    mov    r3, #22
    str    r3, [r0, #0]
    mvn    r0, #0
...
...
   .weak	__errno_location

A statically linked hello world program which uses mmap too.
As we can see__errno_location is completely gone - which is
semantically wrong - we need functional errno.

00008274 <__GI_mmap>:
    8274:    e92d40b0     push    {r4, r5, r7, lr}
    8278:    e59d5014     ldr    r5, [sp, #20]
    827c:    e1b0ca05     lsls    ip, r5, #20
    8280:    0a000004     beq    8298
    8284:    e320f000     nop    {0}
		          ^^^^^^^^^^
    8288:    e3a03016     mov    r3, #22
    828c:    e5803000     str    r3, [r0]
    8290:    e3e00000     mvn    r0, #0

This in turn is due to a fixup in ARM ld which transforms branch-to-null
into a nop. It is better than crashing but still wrong since errno
handling is removed.

With the patch, errno_location is restored back in test program.

00008274 <__GI_mmap>:
    8274:	e92d40b0 	push	{r4, r5, r7, lr}
    8278:	e59d5014 	ldr	r5, [sp, #20]
    827c:	e1b0ca05 	lsls	ip, r5, #20
    8280:	0a000004 	beq	8298 <__GI_mmap+0x24>
    8284:	eb000010 	bl	82cc <__errno_location>
    8288:	e3a03016 	mov	r3, #22
    828c:	e5803000 	str	r3, [r0]

Cc: Christian Ruppert <christian.ruppert@abilis.com>
CC: Francois Bedard <Francois.Bedard@synopsys.com>
Cc: Anton Kolesov <Anton.Kolesov@synopsys.com>
Cc: Joern Rennecke  <joern.rennecke@embecosm.com>
Cc: Jeremy Bennett <jeremy.bennett@embecosm.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Peter Korsgaard <peter@korsgaard.com>
Cc: Khem Raj <raj.khem@gmail
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 include/netdb.h                          | 3 ---
 libc/misc/internals/__errno_location.c   | 2 +-
 libc/misc/internals/__h_errno_location.c | 2 +-
 libc/sysdeps/linux/common/bits/errno.h   | 3 ---
 4 files changed, 2 insertions(+), 8 deletions(-)
aldot - April 16, 2014, 12:03 p.m.
Joern, Vineet,

On 16 April 2014 12:36, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
> Patch "LT.old: Make __errno_location/__h_errno_location thread safe"
> uncovered yet another bug with static linking and errno (hopefully this
> is last of them all).
>
> Currently, __errno_location is declared weak but is defined strong.
> While this provides with the desired weak semantics in dso, it
> is subtly broken in static links.
>
> Quoting Joern Rennecke (ARC gcc expert):
>
> | I think the issue is that you declare the function as weak in the
> | header file.  That is a rare instance where you want the reference
> | use declaration that differs a bit from the definition.
> | If the reference uses a weakly declared function, that creates a
> | weakref, i.e. the linker won't bother to look for this symbol at
> |  all - if it gets linked in for some other reason, fine,
> | otherwise, it stays zero.

Sounds like this is, once again, http://gcc.gnu.org/PR36282 ;
see patch referenced in that PR and the reason for our not_null_ptr()
workaround, no?

thanks,

>
> So the solution to declare strong, define weak.
>
> Supporting data
> -----------------
> orig code: ARM mmap wrapper (LT.old build + my prev patch for errno)
>
> _mmap:
>     @ args = 8, pretend = 0, frame = 0
>     @ frame_needed = 0, uses_anonymous_args = 0
>     stmfd    sp!, {r4, r5, r7, lr}
>     ldr    r5, [sp, #20]
>     movs    ip, r5, asl #20
>     beq    .L2
>     bl    __errno_location(PLT)
>     mov    r3, #22
>     str    r3, [r0, #0]
>     mvn    r0, #0
> ...
> ...
>    .weak        __errno_location
>
> A statically linked hello world program which uses mmap too.
> As we can see__errno_location is completely gone - which is
> semantically wrong - we need functional errno.
>
> 00008274 <__GI_mmap>:
>     8274:    e92d40b0     push    {r4, r5, r7, lr}
>     8278:    e59d5014     ldr    r5, [sp, #20]
>     827c:    e1b0ca05     lsls    ip, r5, #20
>     8280:    0a000004     beq    8298
>     8284:    e320f000     nop    {0}
>                           ^^^^^^^^^^
>     8288:    e3a03016     mov    r3, #22
>     828c:    e5803000     str    r3, [r0]
>     8290:    e3e00000     mvn    r0, #0
>
> This in turn is due to a fixup in ARM ld which transforms branch-to-null
> into a nop. It is better than crashing but still wrong since errno
> handling is removed.
>
> With the patch, errno_location is restored back in test program.
>
> 00008274 <__GI_mmap>:
>     8274:       e92d40b0        push    {r4, r5, r7, lr}
>     8278:       e59d5014        ldr     r5, [sp, #20]
>     827c:       e1b0ca05        lsls    ip, r5, #20
>     8280:       0a000004        beq     8298 <__GI_mmap+0x24>
>     8284:       eb000010        bl      82cc <__errno_location>
>     8288:       e3a03016        mov     r3, #22
>     828c:       e5803000        str     r3, [r0]
>
> Cc: Christian Ruppert <christian.ruppert@abilis.com>
> CC: Francois Bedard <Francois.Bedard@synopsys.com>
> Cc: Anton Kolesov <Anton.Kolesov@synopsys.com>
> Cc: Joern Rennecke  <joern.rennecke@embecosm.com>
> Cc: Jeremy Bennett <jeremy.bennett@embecosm.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Peter Korsgaard <peter@korsgaard.com>
> Cc: Khem Raj <raj.khem@gmail
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  include/netdb.h                          | 3 ---
>  libc/misc/internals/__errno_location.c   | 2 +-
>  libc/misc/internals/__h_errno_location.c | 2 +-
>  libc/sysdeps/linux/common/bits/errno.h   | 3 ---
>  4 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/include/netdb.h b/include/netdb.h
> index 0f361bb6f662..7ce01c24d8b4 100644
> --- a/include/netdb.h
> +++ b/include/netdb.h
> @@ -58,9 +58,6 @@ __BEGIN_DECLS
>
>  /* Function to get address of global `h_errno' variable.  */
>  extern int *__h_errno_location (void) __THROW __attribute__ ((__const__));
> -#ifdef _LIBC
> -extern int weak_const_function *__h_errno_location(void);
> -#endif
>
>  /* Macros for accessing h_errno from inside libc.  */
>  #ifdef _LIBC
> diff --git a/libc/misc/internals/__errno_location.c b/libc/misc/internals/__errno_location.c
> index 9bbc2d779653..6c359f933d16 100644
> --- a/libc/misc/internals/__errno_location.c
> +++ b/libc/misc/internals/__errno_location.c
> @@ -12,7 +12,7 @@
>  extern int errno;
>  #endif
>
> -int *__errno_location(void)
> +int weak_const_function *__errno_location(void)
>  {
>      return &errno;
>  }
> diff --git a/libc/misc/internals/__h_errno_location.c b/libc/misc/internals/__h_errno_location.c
> index b30859e81f19..c510c8143eae 100644
> --- a/libc/misc/internals/__h_errno_location.c
> +++ b/libc/misc/internals/__h_errno_location.c
> @@ -12,7 +12,7 @@
>  extern int h_errno;
>  #endif
>
> -int *__h_errno_location(void)
> +int weak_const_function *__h_errno_location(void)
>  {
>      return &h_errno;
>  }
> diff --git a/libc/sysdeps/linux/common/bits/errno.h b/libc/sysdeps/linux/common/bits/errno.h
> index 611b8359001a..7c0aeb179a75 100644
> --- a/libc/sysdeps/linux/common/bits/errno.h
> +++ b/libc/sysdeps/linux/common/bits/errno.h
> @@ -42,9 +42,6 @@
>  # ifndef __ASSEMBLER__
>  /* Function to get address of global `errno' variable.  */
>  extern int *__errno_location (void) __THROW __attribute__ ((__const__));
> -#  ifdef _LIBC
> -extern int weak_const_function *__errno_location(void);
> -#  endif
>
>  #  ifdef __UCLIBC_HAS_THREADS__
>  /* When using threads, errno is a per-thread value.  */
> --
> 1.8.3.2
>
> _______________________________________________
> uClibc mailing list
> uClibc@uclibc.org
> http://lists.busybox.net/mailman/listinfo/uclibc
Vineet Gupta - April 16, 2014, 12:15 p.m.
On Wednesday 16 April 2014 05:33 PM, Bernhard Reutner-Fischer wrote:
> Joern, Vineet,
>
> On 16 April 2014 12:36, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
>> > Patch "LT.old: Make __errno_location/__h_errno_location thread safe"
>> > uncovered yet another bug with static linking and errno (hopefully this
>> > is last of them all).
>> >
>> > Currently, __errno_location is declared weak but is defined strong.
>> > While this provides with the desired weak semantics in dso, it
>> > is subtly broken in static links.
>> >
>> > Quoting Joern Rennecke (ARC gcc expert):
>> >
>> > | I think the issue is that you declare the function as weak in the
>> > | header file.  That is a rare instance where you want the reference
>> > | use declaration that differs a bit from the definition.
>> > | If the reference uses a weakly declared function, that creates a
>> > | weakref, i.e. the linker won't bother to look for this symbol at
>> > |  all - if it gets linked in for some other reason, fine,
>> > | otherwise, it stays zero.
> Sounds like this is, once again, http://gcc.gnu.org/PR36282 ;
> see patch referenced in that PR and the reason for our not_null_ptr()
> workaround, no?
>
> thanks,

Did you mean http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32219 instead.

Anyhow, using not_null_pointer() is just band aid - what it will do is not
generate reference to __errno_location (using gcc constant expression elimination)
and preventing a runtime segv. However this is a semantical change to program
since we no longer have the needed __errno_location call itself. User programs
might fail if they rely on errno being set (on say mmap failure)

Joern's proposal above fixes the root cause so NULL ref is not generated in first
place.

-Vineet
aldot - April 23, 2014, 2:05 p.m.
On 16 April 2014 12:36, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
> Patch "LT.old: Make __errno_location/__h_errno_location thread safe"
> uncovered yet another bug with static linking and errno (hopefully this
> is last of them all).
>
> Currently, __errno_location is declared weak but is defined strong.
> While this provides with the desired weak semantics in dso, it
> is subtly broken in static links.
>
> Quoting Joern Rennecke (ARC gcc expert):
>
> | I think the issue is that you declare the function as weak in the
> | header file.  That is a rare instance where you want the reference
> | use declaration that differs a bit from the definition.
> | If the reference uses a weakly declared function, that creates a
> | weakref, i.e. the linker won't bother to look for this symbol at
> |  all - if it gets linked in for some other reason, fine,
> | otherwise, it stays zero.
>
> So the solution to declare strong, define weak.
>
> Supporting data
> -----------------
> orig code: ARM mmap wrapper (LT.old build + my prev patch for errno)
>
> _mmap:
>     @ args = 8, pretend = 0, frame = 0
>     @ frame_needed = 0, uses_anonymous_args = 0
>     stmfd    sp!, {r4, r5, r7, lr}
>     ldr    r5, [sp, #20]
>     movs    ip, r5, asl #20
>     beq    .L2
>     bl    __errno_location(PLT)
>     mov    r3, #22
>     str    r3, [r0, #0]
>     mvn    r0, #0
> ...
> ...
>    .weak        __errno_location
>
> A statically linked hello world program which uses mmap too.
> As we can see__errno_location is completely gone - which is
> semantically wrong - we need functional errno.
>
> 00008274 <__GI_mmap>:
>     8274:    e92d40b0     push    {r4, r5, r7, lr}
>     8278:    e59d5014     ldr    r5, [sp, #20]
>     827c:    e1b0ca05     lsls    ip, r5, #20
>     8280:    0a000004     beq    8298
>     8284:    e320f000     nop    {0}
>                           ^^^^^^^^^^
>     8288:    e3a03016     mov    r3, #22
>     828c:    e5803000     str    r3, [r0]
>     8290:    e3e00000     mvn    r0, #0
>
> This in turn is due to a fixup in ARM ld which transforms branch-to-null
> into a nop. It is better than crashing but still wrong since errno
> handling is removed.
>
> With the patch, errno_location is restored back in test program.
>
> 00008274 <__GI_mmap>:
>     8274:       e92d40b0        push    {r4, r5, r7, lr}
>     8278:       e59d5014        ldr     r5, [sp, #20]
>     827c:       e1b0ca05        lsls    ip, r5, #20
>     8280:       0a000004        beq     8298 <__GI_mmap+0x24>
>     8284:       eb000010        bl      82cc <__errno_location>
>     8288:       e3a03016        mov     r3, #22
>     828c:       e5803000        str     r3, [r0]
>
> Cc: Christian Ruppert <christian.ruppert@abilis.com>
> CC: Francois Bedard <Francois.Bedard@synopsys.com>
> Cc: Anton Kolesov <Anton.Kolesov@synopsys.com>
> Cc: Joern Rennecke  <joern.rennecke@embecosm.com>
> Cc: Jeremy Bennett <jeremy.bennett@embecosm.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Peter Korsgaard <peter@korsgaard.com>
> Cc: Khem Raj <raj.khem@gmail
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>

Applied, thanks!

Patch

diff --git a/include/netdb.h b/include/netdb.h
index 0f361bb6f662..7ce01c24d8b4 100644
--- a/include/netdb.h
+++ b/include/netdb.h
@@ -58,9 +58,6 @@  __BEGIN_DECLS
 
 /* Function to get address of global `h_errno' variable.  */
 extern int *__h_errno_location (void) __THROW __attribute__ ((__const__));
-#ifdef _LIBC
-extern int weak_const_function *__h_errno_location(void);
-#endif
 
 /* Macros for accessing h_errno from inside libc.  */
 #ifdef _LIBC
diff --git a/libc/misc/internals/__errno_location.c b/libc/misc/internals/__errno_location.c
index 9bbc2d779653..6c359f933d16 100644
--- a/libc/misc/internals/__errno_location.c
+++ b/libc/misc/internals/__errno_location.c
@@ -12,7 +12,7 @@ 
 extern int errno;
 #endif
 
-int *__errno_location(void)
+int weak_const_function *__errno_location(void)
 {
     return &errno;
 }
diff --git a/libc/misc/internals/__h_errno_location.c b/libc/misc/internals/__h_errno_location.c
index b30859e81f19..c510c8143eae 100644
--- a/libc/misc/internals/__h_errno_location.c
+++ b/libc/misc/internals/__h_errno_location.c
@@ -12,7 +12,7 @@ 
 extern int h_errno;
 #endif
 
-int *__h_errno_location(void)
+int weak_const_function *__h_errno_location(void)
 {
     return &h_errno;
 }
diff --git a/libc/sysdeps/linux/common/bits/errno.h b/libc/sysdeps/linux/common/bits/errno.h
index 611b8359001a..7c0aeb179a75 100644
--- a/libc/sysdeps/linux/common/bits/errno.h
+++ b/libc/sysdeps/linux/common/bits/errno.h
@@ -42,9 +42,6 @@ 
 # ifndef __ASSEMBLER__
 /* Function to get address of global `errno' variable.  */
 extern int *__errno_location (void) __THROW __attribute__ ((__const__));
-#  ifdef _LIBC
-extern int weak_const_function *__errno_location(void);
-#  endif
 
 #  ifdef __UCLIBC_HAS_THREADS__
 /* When using threads, errno is a per-thread value.  */