mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] errno and swapcontext in a multithreaded setup
@ 2021-04-08 16:10 Andrey Bugaevskiy
  2021-04-08 17:04 ` Florian Weimer
  2021-04-08 17:40 ` Ariadne Conill
  0 siblings, 2 replies; 9+ messages in thread
From: Andrey Bugaevskiy @ 2021-04-08 16:10 UTC (permalink / raw)
  To: musl

Hi,

I'm using makecontext/swapcontext to migrate contexts between threads
and this sometimes leads to getting incorrect errno values.

Investigating further I've noticed that __errno_location
is marked __attribute__((const)).
This causes optimizers to assume that errno address never changes
in the scope of the function which is not the case in my scenario.

Namely, this code:

int test(ucontext_t* old_ctx, const ucontext_t* new_ctx) {
    int err_before = errno;
    swapcontext(old_ctx, new_ctx);
    int err_after = errno;
    return err_before | err_after; // do not optimize out
}

translates with -O1 to something like this:

0000000000001109 <test>:
1109: endbr64
110d: push %r13
110f: push %r12
1111: push %rbp
1112: push %rbx
1113: sub $0x8,%rsp
1117: mov %rdi,%r12
111a: mov %rsi,%r13
111d: callq 1030 <__errno_location@plt>
1122: mov %rax,%rbx
1125: mov (%rax),%ebp
1127: mov %r13,%rsi
112a: mov %r12,%rdi
112d: callq 1020 <swapcontext@plt>
1132: mov %ebp,%eax
1134: or (%rbx),%eax
1136: add $0x8,%rsp
113a: pop %rbx
113b: pop %rbp
113c: pop %r12
113e: pop %r13
1140: retq

errno location is being stored to a register and then reused.
However a call to __errno_location after swapcontext is expected to
return a different address if the context have been swapped back
into another thread.
There are a couple of similarly affected functions (pthread_self,
__h_errno_location).

Removing __attribute__((const)) or changing it
to __attribute__((pure)) resolves the problem in newly compiled code.

Can this change be considered for the future versions of musl?

-- 
Andrey Bugaevskiy


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] errno and swapcontext in a multithreaded setup
  2021-04-08 16:10 [musl] errno and swapcontext in a multithreaded setup Andrey Bugaevskiy
@ 2021-04-08 17:04 ` Florian Weimer
  2021-04-08 17:17   ` Rich Felker
  2021-04-08 17:46   ` Andrey Bugaevskiy
  2021-04-08 17:40 ` Ariadne Conill
  1 sibling, 2 replies; 9+ messages in thread
From: Florian Weimer @ 2021-04-08 17:04 UTC (permalink / raw)
  To: Andrey Bugaevskiy; +Cc: musl

* Andrey Bugaevskiy:

> I'm using makecontext/swapcontext to migrate contexts between threads
> and this sometimes leads to getting incorrect errno values.
>
> Investigating further I've noticed that __errno_location
> is marked __attribute__((const)).
> This causes optimizers to assume that errno address never changes
> in the scope of the function which is not the case in my scenario.

The optimizers make the same assumption for actual thread-local
variables, not just __errno_location.  Migrating contexts between
threads results in undefined behavior.

Thanks,
Florian


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] errno and swapcontext in a multithreaded setup
  2021-04-08 17:04 ` Florian Weimer
@ 2021-04-08 17:17   ` Rich Felker
  2021-04-08 17:46   ` Andrey Bugaevskiy
  1 sibling, 0 replies; 9+ messages in thread
From: Rich Felker @ 2021-04-08 17:17 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Andrey Bugaevskiy, musl

On Thu, Apr 08, 2021 at 07:04:09PM +0200, Florian Weimer wrote:
> * Andrey Bugaevskiy:
> 
> > I'm using makecontext/swapcontext to migrate contexts between threads
> > and this sometimes leads to getting incorrect errno values.
> >
> > Investigating further I've noticed that __errno_location
> > is marked __attribute__((const)).
> > This causes optimizers to assume that errno address never changes
> > in the scope of the function which is not the case in my scenario.
> 
> The optimizers make the same assumption for actual thread-local
> variables, not just __errno_location.  Migrating contexts between
> threads results in undefined behavior.

Indeed, this is not functionality that has ever been defined on any
implementation I'm aware of. There are a lot of other things that
could go wrong. Even if you don't explicitly use TLS, if your code is
built with stack protector it may access the canary value from TLS,
and the value may be individual to each thread (musl doesn't currently
do this, but could in the future as further hardening).

Rich

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] errno and swapcontext in a multithreaded setup
  2021-04-08 16:10 [musl] errno and swapcontext in a multithreaded setup Andrey Bugaevskiy
  2021-04-08 17:04 ` Florian Weimer
@ 2021-04-08 17:40 ` Ariadne Conill
  2021-04-08 17:49   ` Rich Felker
  1 sibling, 1 reply; 9+ messages in thread
From: Ariadne Conill @ 2021-04-08 17:40 UTC (permalink / raw)
  To: musl

Hello,

On Thu, 8 Apr 2021, Andrey Bugaevskiy wrote:

> Hi,
>
> I'm using makecontext/swapcontext to migrate contexts between threads
> and this sometimes leads to getting incorrect errno values.

This is not supported by libucontext.

> Investigating further I've noticed that __errno_location
> is marked __attribute__((const)).
> This causes optimizers to assume that errno address never changes
> in the scope of the function which is not the case in my scenario.
>
> Namely, this code:
>
> int test(ucontext_t* old_ctx, const ucontext_t* new_ctx) {
>    int err_before = errno;
>    swapcontext(old_ctx, new_ctx);
>    int err_after = errno;
>    return err_before | err_after; // do not optimize out
> }
>
> translates with -O1 to something like this:
>
> 0000000000001109 <test>:
> 1109: endbr64
> 110d: push %r13
> 110f: push %r12
> 1111: push %rbp
> 1112: push %rbx
> 1113: sub $0x8,%rsp
> 1117: mov %rdi,%r12
> 111a: mov %rsi,%r13
> 111d: callq 1030 <__errno_location@plt>
> 1122: mov %rax,%rbx
> 1125: mov (%rax),%ebp
> 1127: mov %r13,%rsi
> 112a: mov %r12,%rdi
> 112d: callq 1020 <swapcontext@plt>
> 1132: mov %ebp,%eax
> 1134: or (%rbx),%eax
> 1136: add $0x8,%rsp
> 113a: pop %rbx
> 113b: pop %rbp
> 113c: pop %r12
> 113e: pop %r13
> 1140: retq
>
> errno location is being stored to a register and then reused.
> However a call to __errno_location after swapcontext is expected to
> return a different address if the context have been swapped back
> into another thread.
> There are a couple of similarly affected functions (pthread_self,
> __h_errno_location).
>
> Removing __attribute__((const)) or changing it
> to __attribute__((pure)) resolves the problem in newly compiled code.

I believe these values use TLS (but not 100% sure, at least in uClibc 
they do).  libucontext is not aware of TLS, and so clobbers the TLS 
register.  You could modify libucontext to add awareness of the TLS 
register, but I don't think we would accept a patch for that.

> Can this change be considered for the future versions of musl?

No, musl is working as designed here.

Ariadne

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [musl] errno and swapcontext in a multithreaded setup
  2021-04-08 17:04 ` Florian Weimer
  2021-04-08 17:17   ` Rich Felker
@ 2021-04-08 17:46   ` Andrey Bugaevskiy
  2021-04-08 18:55     ` Rich Felker
  1 sibling, 1 reply; 9+ messages in thread
From: Andrey Bugaevskiy @ 2021-04-08 17:46 UTC (permalink / raw)
  To: 'Florian Weimer'; +Cc: musl

On Thu, 8 Apr 2021, Florian Weimer wrote:
> * Andrey Bugaevskiy:
> 
> > I'm using makecontext/swapcontext to migrate contexts between threads
> > and this sometimes leads to getting incorrect errno values.
> >
> > Investigating further I've noticed that __errno_location
> > is marked __attribute__((const)).
> > This causes optimizers to assume that errno address never changes
> > in the scope of the function which is not the case in my scenario.
> 
> The optimizers make the same assumption for actual thread-local
> variables, not just __errno_location.  Migrating contexts between
> threads results in undefined behavior.

Can you please point me to some explanation why this optimization
is valid for thread-local variables?

As far as I can imagine optimizer should at least prove that it
won't be changed from some other place or (if the variable is local
to the function) that it is not changed by a recursive call.

I believe the only reason optimizer is allowed to remove the call
to __errno_location is the "const" attribute.

--
Andrey Bugaevskiy


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] errno and swapcontext in a multithreaded setup
  2021-04-08 17:40 ` Ariadne Conill
@ 2021-04-08 17:49   ` Rich Felker
  0 siblings, 0 replies; 9+ messages in thread
From: Rich Felker @ 2021-04-08 17:49 UTC (permalink / raw)
  To: Ariadne Conill; +Cc: musl

On Thu, Apr 08, 2021 at 11:40:43AM -0600, Ariadne Conill wrote:
> Hello,
> 
> On Thu, 8 Apr 2021, Andrey Bugaevskiy wrote:
> 
> >Hi,
> >
> >I'm using makecontext/swapcontext to migrate contexts between threads
> >and this sometimes leads to getting incorrect errno values.
> 
> This is not supported by libucontext.
> 
> >Investigating further I've noticed that __errno_location
> >is marked __attribute__((const)).
> >This causes optimizers to assume that errno address never changes
> >in the scope of the function which is not the case in my scenario.
> >
> >Namely, this code:
> >
> >int test(ucontext_t* old_ctx, const ucontext_t* new_ctx) {
> >   int err_before = errno;
> >   swapcontext(old_ctx, new_ctx);
> >   int err_after = errno;
> >   return err_before | err_after; // do not optimize out
> >}
> >
> >translates with -O1 to something like this:
> >
> >0000000000001109 <test>:
> >1109: endbr64
> >110d: push %r13
> >110f: push %r12
> >1111: push %rbp
> >1112: push %rbx
> >1113: sub $0x8,%rsp
> >1117: mov %rdi,%r12
> >111a: mov %rsi,%r13
> >111d: callq 1030 <__errno_location@plt>
> >1122: mov %rax,%rbx
> >1125: mov (%rax),%ebp
> >1127: mov %r13,%rsi
> >112a: mov %r12,%rdi
> >112d: callq 1020 <swapcontext@plt>
> >1132: mov %ebp,%eax
> >1134: or (%rbx),%eax
> >1136: add $0x8,%rsp
> >113a: pop %rbx
> >113b: pop %rbp
> >113c: pop %r12
> >113e: pop %r13
> >1140: retq
> >
> >errno location is being stored to a register and then reused.
> >However a call to __errno_location after swapcontext is expected to
> >return a different address if the context have been swapped back
> >into another thread.
> >There are a couple of similarly affected functions (pthread_self,
> >__h_errno_location).
> >
> >Removing __attribute__((const)) or changing it
> >to __attribute__((pure)) resolves the problem in newly compiled code.
> 
> I believe these values use TLS (but not 100% sure, at least in
> uClibc they do).  libucontext is not aware of TLS, and so clobbers
> the TLS register.  You could modify libucontext to add awareness of
> the TLS register, but I don't think we would accept a patch for
> that.

Note that whether the "TLS register" is swapped or preserved is partly
a function of whether it's userspace or kernelspace. For example on
powerpc where it's in a general purpose register, the ucontext
functions could be swapping it (but IMO shouldn't; this would break
even worse by changing the "identity" of the thread when a programmer
commits this kind of UB). On x86 where it's kernel state in segment
descriptors (same descriptor value refers to a different segment in
each thread context) they really can't swap it without a lot of
hackery.

Rich

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] errno and swapcontext in a multithreaded setup
  2021-04-08 17:46   ` Andrey Bugaevskiy
@ 2021-04-08 18:55     ` Rich Felker
  2021-04-09 12:11       ` Andrey Bugaevskiy
  0 siblings, 1 reply; 9+ messages in thread
From: Rich Felker @ 2021-04-08 18:55 UTC (permalink / raw)
  To: Andrey Bugaevskiy; +Cc: 'Florian Weimer', musl

On Thu, Apr 08, 2021 at 08:46:00PM +0300, Andrey Bugaevskiy wrote:
> On Thu, 8 Apr 2021, Florian Weimer wrote:
> > * Andrey Bugaevskiy:
> > 
> > > I'm using makecontext/swapcontext to migrate contexts between threads
> > > and this sometimes leads to getting incorrect errno values.
> > >
> > > Investigating further I've noticed that __errno_location
> > > is marked __attribute__((const)).
> > > This causes optimizers to assume that errno address never changes
> > > in the scope of the function which is not the case in my scenario.
> > 
> > The optimizers make the same assumption for actual thread-local
> > variables, not just __errno_location.  Migrating contexts between
> > threads results in undefined behavior.
> 
> Can you please point me to some explanation why this optimization
> is valid for thread-local variables?
> 
> As far as I can imagine optimizer should at least prove that it
> won't be changed from some other place or (if the variable is local
> to the function) that it is not changed by a recursive call.

Perhaps you're confusing the value of errno with its address. No
function can change the address of errno, only its value.
Conceptually, __errno_location() is just &errno.

Rich

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [musl] errno and swapcontext in a multithreaded setup
  2021-04-08 18:55     ` Rich Felker
@ 2021-04-09 12:11       ` Andrey Bugaevskiy
  2021-04-09 12:15         ` 'Rich Felker'
  0 siblings, 1 reply; 9+ messages in thread
From: Andrey Bugaevskiy @ 2021-04-09 12:11 UTC (permalink / raw)
  To: 'Rich Felker'; +Cc: 'Florian Weimer', musl

On Thu, 8 Apr 2021, Rich Felker wrote:
> On Thu, Apr 08, 2021 at 08:46:00PM +0300, Andrey Bugaevskiy wrote:
> > On Thu, 8 Apr 2021, Florian Weimer wrote:
> > > * Andrey Bugaevskiy:
> > > 
> > > > I'm using makecontext/swapcontext to migrate contexts between
threads
> > > > and this sometimes leads to getting incorrect errno values.
> > > >
> > > > Investigating further I've noticed that __errno_location
> > > > is marked __attribute__((const)).
> > > > This causes optimizers to assume that errno address never changes
> > > > in the scope of the function which is not the case in my scenario.
> > > 
> > > The optimizers make the same assumption for actual thread-local
> > > variables, not just __errno_location.  Migrating contexts between
> > > threads results in undefined behavior.
> > 
> > Can you please point me to some explanation why this optimization
> > is valid for thread-local variables?
> > 
> > As far as I can imagine optimizer should at least prove that it
> > won't be changed from some other place or (if the variable is local
> > to the function) that it is not changed by a recursive call.
> 
> Perhaps you're confusing the value of errno with its address. No
> function can change the address of errno, only its value.
> Conceptually, __errno_location() is just &errno.

Well, it's a function returning a value, but I see the point.
I'm still a bit confused about why optimizer can assume that
TLS location never changes though.

Thanks to everyone for their replies.

--
Andrey Bugaevskiy


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] errno and swapcontext in a multithreaded setup
  2021-04-09 12:11       ` Andrey Bugaevskiy
@ 2021-04-09 12:15         ` 'Rich Felker'
  0 siblings, 0 replies; 9+ messages in thread
From: 'Rich Felker' @ 2021-04-09 12:15 UTC (permalink / raw)
  To: Andrey Bugaevskiy; +Cc: 'Florian Weimer', musl

On Fri, Apr 09, 2021 at 03:11:14PM +0300, Andrey Bugaevskiy wrote:
> On Thu, 8 Apr 2021, Rich Felker wrote:
> > On Thu, Apr 08, 2021 at 08:46:00PM +0300, Andrey Bugaevskiy wrote:
> > > On Thu, 8 Apr 2021, Florian Weimer wrote:
> > > > * Andrey Bugaevskiy:
> > > > 
> > > > > I'm using makecontext/swapcontext to migrate contexts between
> threads
> > > > > and this sometimes leads to getting incorrect errno values.
> > > > >
> > > > > Investigating further I've noticed that __errno_location
> > > > > is marked __attribute__((const)).
> > > > > This causes optimizers to assume that errno address never changes
> > > > > in the scope of the function which is not the case in my scenario.
> > > > 
> > > > The optimizers make the same assumption for actual thread-local
> > > > variables, not just __errno_location.  Migrating contexts between
> > > > threads results in undefined behavior.
> > > 
> > > Can you please point me to some explanation why this optimization
> > > is valid for thread-local variables?
> > > 
> > > As far as I can imagine optimizer should at least prove that it
> > > won't be changed from some other place or (if the variable is local
> > > to the function) that it is not changed by a recursive call.
> > 
> > Perhaps you're confusing the value of errno with its address. No
> > function can change the address of errno, only its value.
> > Conceptually, __errno_location() is just &errno.
> 
> Well, it's a function returning a value, but I see the point.
> I'm still a bit confused about why optimizer can assume that
> TLS location never changes though.

Because that's a guarantee of the C language. All objects have
addresses that are constant for their lifetime. For example, if *you*
took &errno or &some_tls_var and saved it, the address you saved is
valid until the lifetime of the object ends. It will compare equal to
what you get if you take the address again. (The fact tht it doesn't
necessarily in your ucontext example is a consequence of you having
invoked UB by attempting to "move" an executing block from one thread
to another.)

Rich

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-09 12:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08 16:10 [musl] errno and swapcontext in a multithreaded setup Andrey Bugaevskiy
2021-04-08 17:04 ` Florian Weimer
2021-04-08 17:17   ` Rich Felker
2021-04-08 17:46   ` Andrey Bugaevskiy
2021-04-08 18:55     ` Rich Felker
2021-04-09 12:11       ` Andrey Bugaevskiy
2021-04-09 12:15         ` 'Rich Felker'
2021-04-08 17:40 ` Ariadne Conill
2021-04-08 17:49   ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).