is stdlib/tst-setcontext7 a bad test?

[Szabolcs Nagy]

The 03/27/2020 06:16, H.J. Lu wrote:
> On Fri, Mar 27, 2020 at 5:49 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Fri, Mar 27, 2020 at 5:46 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Fri, Mar 27, 2020 at 4:04 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > >
> > > > The 03/27/2020 11:10, Florian Weimer wrote:
> > > > > * Szabolcs Nagy:
> > > > > > i'm planing to change swapcontext on aarch64 and i see
> > > > > >
> > > > > > FAIL: stdlib/tst-setcontext7
> > > > > >
> > > > > > that test case seems wrong to me, a simplified version is
> > > > > >
> > > > > > #include <ucontext.h>
> > > > > > #include <stdio.h>
> > > > > >
> > > > > > ucontext_t uc[2];
> > > > > > volatile int count = 0;
> > > > > >
> > > > > > int main ()
> > > > > > {
> > > > > >     getcontext (uc+0);
> > > > > >     printf ("%d\n", count);
> > > > > >     if (count)
> > > > > >             setcontext (uc+1);
> > > > > >     count++;
> > > > > >     swapcontext (uc+1, uc+0);
> > > > > >     printf ("done\n");
> > > > > >     return 0;
> > > > > > }
> > > > > >
> > > > > > this seems to work today and prints
> > > > > >
> > > > > > 0
> > > > > > 1
> > > > > > done
> > > > > >
> > > > > > but i don't think we can guarantee that after a
> > > > > > swapcontext to a getcontext on the same stack frame
> > > > > > resuming the context works: if swapcontext uses the
> > > > > > stack (which is what i plan to do) then that stack
> > > > > > will be corrupted.
> > > > > >
> > > > > > does glibc plan to support this usage?
> > > > >
> > > > > I think it's expected to work.  This seems to be one of the more
> > > > > harmless cases.
> > > > >
> > > > > > i.e. should i ensure swapcontext does not use the stack? (and should
> > > > > > gcc ensure no stack is used in the example between getcontext and
> > > > > > swapcontext?)
> > > > >
> > > > > Does the test case pass if you add the returns_twice attribute (or
> > > > > __INDIRECT_RETURN) to getcontext as well (swapcontext already has it)?
> > > >
> > > > yes, the returns_twice attribute on swapcontext
> > > > makes bti work, but that seems ugly (swapcontext
> > > > only returns once).
> > > >
> > > > (getcontext has the return_twice attribute because
> > > > it's a gcc builtin.)
> > > >
> > > > the way i planed to handle swapcontext is to do
> > > >
> > > > swapcontext:
> > > >         bti c
> > > >         stp x29, x30, [sp, #-16]! // save return address
> > > >         bl internal_swapcontext
> > > >         bti j
> > > >         ldp x29, x30, [sp], #16 // restore return address
> > > >         ret
> > > >
> > > > then bti j (landing pad for indirect jump) is
> > > > in libc code, otherwise the compiler has to
> > > > special case swapcontext and emit a bti j after
> > > > every call site (getcontext is already special),
> > > > with my solution swapcontext is not special.
> > > >
> > > > but if glibc plans to support jumping around
> > > > within the same stack frame then my solution
> > > > does not work so i will have to rely on the
> > > > attribute.
> > > >
> > >
> > > Can't you use indirect_return function attribute like the fix for
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85620
> > >
> >
> > You should also check if c-c++-common/asan/swapcontext-test-1.c
> > is OK:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86560
> >
> 
> You also need to provide bits/indirect-return.h for
> 
> /* __INDIRECT_RETURN is used on swapcontext to indicate if it requires
>    special compiler treatment.  */
> #define __INDIRECT_RETURN
> 
> See sysdeps/x86/bits/indirect-return.h for example.

thanks for the heads up.

i've seen bits/indirec-return.h. i would still prefer
not to special case swapcontext, that can avoid all
sorts of bugs and compatible with existing compilers.

the context switch to a place on the same stack and
back is what seems unsupportable to me: it requires
the compiler to know the callers of swapcontext and
generate code specially: this is already broken on
all targets without special annotation, since there
is nothing preventing the compiler to use the stack
in ways that breaks such usage.

if we want to fix this then the attribute has to be
introduced across all targets and gcc has to handle
that in a generic way (i.e. not conditionally adding
the attribute in libc only when cet/bti is enabled)

but that's a libc api change (changing the type of
swapcontext) affecting users who take the address
of swapcontext.

it's not clear to me if going the x86 way is more
future proof or just no longer supporting usage
like stdlib/tst-setcontext7.