Patchwork LT.old: Make errno_location thread safe

login
register
mail settings
Submitter Vineet Gupta
Date March 12, 2014, 2:05 p.m.
Message ID <1394633133-15657-1-git-send-email-vgupta@synopsys.com>
Download mbox | patch
Permalink /patch/329470/
State Superseded
Headers show

Comments

Vineet Gupta - March 12, 2014, 2:05 p.m.
[summary: Get rid of libc alias __GI___errno_location]

It seems with Linuxthreads.old (yet to confirm NPTL) errno is not thread
safe.

A simple pthread linked test program (at the bottom) which makes a
failing syscall e.g. open("/not-exist") fails to observe the right errno
in the thread (main is OK)

Conceptually uClibc defines weak __errno_location() while libpthread
defines astrong variant. This arrangement shd work when using -pthread
links. The spoil sport is __GI___errno_location, intended to bypass PLT
for intra-libc callers. It gets called even in case of LT.old links
given the syscall wrappers in  libpthread (LT.old). e.g.

open [ in libpthread ]
  pthreadsetcanceltype()
   __libc_open()
      __GI__open()
           ...
-           __GI___errno_location    [ existing ]
+           __errno_location         [ intended ]
   ....
  pthreadsetcanceltype()

So the solution is to get rid of GI alias for errno_location
altogether.

I tested this on master for ARC as well as an ARMv7 buildroot LT.old
build (daily snapshot of uClibc) and fix works on both.

-------- before ------->8----------

[Main thread] Initial errno = 99
errno @b6f6c338 = 2 (OK)

[Child Thread] Initial errno = 0
errno @be1ffee8 = 0 (BROKEN)
Errno in main after child 4

------- after ----->8------------------

[Main thread] Initial errno = 99
errno @b6f9a338 = 2 (OK)

[Child Thread] Initial errno = 0
errno @be1ffee8 = 2 (OK)
Errno in main after child 4
Peter Korsgaard - March 12, 2014, 2:12 p.m.
>>>>> "Vineet" == Vineet Gupta <Vineet.Gupta1@synopsys.com> writes:

 > [summary: Get rid of libc alias __GI___errno_location]
 > It seems with Linuxthreads.old (yet to confirm NPTL) errno is not thread
 > safe.

 > A simple pthread linked test program (at the bottom) which makes a
 > failing syscall e.g. open("/not-exist") fails to observe the right errno
 > in the thread (main is OK)

 > Conceptually uClibc defines weak __errno_location() while libpthread
 > defines astrong variant. This arrangement shd work when using -pthread
 > links. The spoil sport is __GI___errno_location, intended to bypass PLT
 > for intra-libc callers. It gets called even in case of LT.old links
 > given the syscall wrappers in  libpthread (LT.old). e.g.

 > open [ in libpthread ]
 >   pthreadsetcanceltype()
 >    __libc_open()
 >       __GI__open()
 >            ...
 > -           __GI___errno_location    [ existing ]
 > +           __errno_location         [ intended ]
 >    ....
 >   pthreadsetcanceltype()

 > So the solution is to get rid of GI alias for errno_location
 > altogether.

That sounds pretty much like:

http://lists.uclibc.org/pipermail/uclibc/2010-July/044176.html

Which afaik never got committed.
Vineet Gupta - March 12, 2014, 2:20 p.m.
On Wednesday 12 March 2014 07:42 PM, Peter Korsgaard wrote:
> That sounds pretty much like:
> http://lists.uclibc.org/pipermail/uclibc/2010-July/044176.html Which afaik never
> got committed. 

Indeed and the changelog is one million times better than mine :-)

So looking back at the orig bug log, it seems it was concluded that fix was legit
so why was it not merged - @Bernhard you made a last comment 'This works with
NPTL" but it is broken in LT.old

Thx,
-Vineet
aldot - March 12, 2014, 8:03 p.m.
On Wed, Mar 12, 2014 at 02:20:20PM +0000, Vineet Gupta wrote:
> On Wednesday 12 March 2014 07:42 PM, Peter Korsgaard wrote:
> > That sounds pretty much like:
> > http://lists.uclibc.org/pipermail/uclibc/2010-July/044176.html Which afaik never
> > got committed. 
> 
> Indeed and the changelog is one million times better than mine :-)
> 
> So looking back at the orig bug log, it seems it was concluded that fix was legit
> so why was it not merged - @Bernhard you made a last comment 'This works with
> NPTL" but it is broken in LT.old

Abovementioned patch was applied by Austin but quickly reverted since
it broke static (IIRC) use.
Vineet Gupta - March 13, 2014, 4:25 a.m.
On Thursday 13 March 2014 01:33 AM, Bernhard Reutner-Fischer wrote:
> On Wed, Mar 12, 2014 at 02:20:20PM +0000, Vineet Gupta wrote:
>> On Wednesday 12 March 2014 07:42 PM, Peter Korsgaard wrote:
>>> That sounds pretty much like:
>>> http://lists.uclibc.org/pipermail/uclibc/2010-July/044176.html Which afaik never
>>> got committed. 
>> Indeed and the changelog is one million times better than mine :-)
>>
>> So looking back at the orig bug log, it seems it was concluded that fix was legit
>> so why was it not merged - @Bernhard you made a last comment 'This works with
>> NPTL" but it is broken in LT.old
> Abovementioned patch was applied by Austin but quickly reverted since
> it broke static (IIRC) use.

I forgot to mention that I've tested static links on ARC as well as ARM.
That breakage must be due to an earlier issue I fixed where __errno_location was
not weak in libc hence would collide when linking with libpthread

-Vineet
Vineet Gupta - April 1, 2014, 5:22 a.m.
On Thursday 13 March 2014 09:55 AM, Vineet Gupta wrote:
> On Thursday 13 March 2014 01:33 AM, Bernhard Reutner-Fischer wrote:
>> On Wed, Mar 12, 2014 at 02:20:20PM +0000, Vineet Gupta wrote:
>>> On Wednesday 12 March 2014 07:42 PM, Peter Korsgaard wrote:
>>>> That sounds pretty much like:
>>>> http://lists.uclibc.org/pipermail/uclibc/2010-July/044176.html Which afaik never
>>>> got committed. 
>>> Indeed and the changelog is one million times better than mine :-)
>>>
>>> So looking back at the orig bug log, it seems it was concluded that fix was legit
>>> so why was it not merged - @Bernhard you made a last comment 'This works with
>>> NPTL" but it is broken in LT.old
>> Abovementioned patch was applied by Austin but quickly reverted since
>> it broke static (IIRC) use.
> 
> I forgot to mention that I've tested static links on ARC as well as ARM.
> That breakage must be due to an earlier issue I fixed where __errno_location was
> not weak in libc hence would collide when linking with libpthread
> 
> -Vineet
> 


Ping !

Patch

============Test case ===================
/* $CROSS-gcc -o errno-arc -lpthread */

void *th(void *str)
{
	int fd;
	printf("\n[%s] Initial errno = %d\n", (char *)str, errno);

	fd = open("/nothing", O_RDONLY);
	if (fd >= 0) {
		close(fd);
		//printf("success\n");
	} else
		printf("errno @%x = %d (%s)\n", &errno, errno, errno ?  "OK" : "BROKEN");

	errno = 66;

	return NULL;
}

int main(void)
{
	pthread_t t;
	void *r;
	int err;

	errno = 99;
	th("Main thread");

	err = pthread_create(&t, NULL, th, "Child Thread");
	if (err) {
		fprintf(stderr, "could not spawn thread: %s\n", strerror(err));
		return 1;
	}
	pthread_join(t, &r);

	printf("Errno in main after child %d\n", errno);
	return 0;
}

Cc: Christian Ruppert <christian.ruppert@abilis.com>
CC: Francois Bedard <Francois.Bedard@synopsys.com>
Cc: Joern Rennecke  <joern.rennecke@embecosm.com>
Cc: Jeremy Bennett <jeremy.bennett@embecosm.com>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 libc/misc/internals/__errno_location.c | 1 -
 libc/sysdeps/linux/common/bits/errno.h | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/libc/misc/internals/__errno_location.c b/libc/misc/internals/__errno_location.c
index be7a9093efa3..9bbc2d779653 100644
--- a/libc/misc/internals/__errno_location.c
+++ b/libc/misc/internals/__errno_location.c
@@ -16,4 +16,3 @@  int *__errno_location(void)
 {
     return &errno;
 }
-libc_hidden_weak(__errno_location)
diff --git a/libc/sysdeps/linux/common/bits/errno.h b/libc/sysdeps/linux/common/bits/errno.h
index 777338fb1e0a..611b8359001a 100644
--- a/libc/sysdeps/linux/common/bits/errno.h
+++ b/libc/sysdeps/linux/common/bits/errno.h
@@ -43,11 +43,8 @@ 
 /* Function to get address of global `errno' variable.  */
 extern int *__errno_location (void) __THROW __attribute__ ((__const__));
 #  ifdef _LIBC
-#   if !defined(__UCLIBC_HAS_TLS__) && !defined(__UCLIBC_HAS_THREADS__)
 extern int weak_const_function *__errno_location(void);
-#   endif
 #  endif
-libc_hidden_proto(__errno_location)
 
 #  ifdef __UCLIBC_HAS_THREADS__
 /* When using threads, errno is a per-thread value.  */