PATCH: don't call cleanup handlers after a regular return from the thread start function

PATCH: don't call cleanup handlers after a regular return from the thread start function

[Jens Gustedt]

[-- Attachment #1.1: Type: text/plain, Size: 1033 bytes --]

Don't call cleanup handlers after a regular return from the thread
start function

The chained list of cleanup handler function uses list items that are
local to the respective function of a cleanup block. In case of a
return out of the middle of a cleanup block, using these list items
can lead to UB.

POSIX lists three different cases in which a cleanup handler that is
established on the cleanup stack has to be executed. Regular return
from the thread start function is not among these cases.

Linux manpages are more explicit and state:

    Clean-up handlers are not called if the thread terminates by
    performing a return from the thread start function.

This patch aligns musl to that behavior.


-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::



[-- Attachment #1.2: cleanup-push-fix.patch --]
[-- Type: text/x-patch, Size: 859 bytes --]

diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
index e77e54a..8441845 100644
--- a/src/thread/pthread_create.c
+++ b/src/thread/pthread_create.c
@@ -97,6 +97,7 @@ void __do_cleanup_pop(struct __ptcb *cb)
 
 static int start(void *p)
 {
+	void* ret;
 	pthread_t self = p;
 	if (self->startlock[0]) {
 		__wait(self->startlock, 0, 1, 1);
@@ -109,7 +110,12 @@ static int start(void *p)
 	if (self->unblock_cancel)
 		__syscall(SYS_rt_sigprocmask, SIG_UNBLOCK,
 			SIGPT_SET, 0, _NSIG/8);
-	pthread_exit(self->start(self->start_arg));
+	ret = self->start(self->start_arg);
+	/* POSIX states: The thread exits (that is, calls pthread_exit())
+	   According to the documentation on Linux a return from the
+	   function doesn't count as such an exit. */
+	self->cancelbuf = 0;
+	pthread_exit(ret);
 	return 0;
 }
 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Rich Felker]

On Tue, Aug 05, 2014 at 06:51:34PM +0200, Jens Gustedt wrote:
> Don't call cleanup handlers after a regular return from the thread
> start function
> 
> The chained list of cleanup handler function uses list items that are
> local to the respective function of a cleanup block. In case of a
> return out of the middle of a cleanup block, using these list items
> can lead to UB.
> 
> POSIX lists three different cases in which a cleanup handler that is
> established on the cleanup stack has to be executed. Regular return
> from the thread start function is not among these cases.
> 
> Linux manpages are more explicit and state:
> 
>     Clean-up handlers are not called if the thread terminates by
>     performing a return from the thread start function.
> 
> This patch aligns musl to that behavior.

Could you clarify why this patch is necessary? I think such a return
is explicitly UB:

"The effect of the use of return, break, continue, and goto to
prematurely leave a code block described by a pair of
pthread_cleanup_push() and pthread_cleanup_pop() functions calls is
undefined."

http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cleanup_pop.html

I don't see why the thread start function should be treated as special
here.

Rich

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]

Hello,

Am Dienstag, den 05.08.2014, 13:09 -0400 schrieb Rich Felker:
> > Linux manpages are more explicit and state:
> > 
> >     Clean-up handlers are not called if the thread terminates by
> >     performing a return from the thread start function.
> > 
> > This patch aligns musl to that behavior.
> 
> Could you clarify why this patch is necessary? I think such a return
> is explicitly UB.
> 
> "The effect of the use of return, break, continue, and goto to
> prematurely leave a code block described by a pair of
> pthread_cleanup_push() and pthread_cleanup_pop() functions calls is
> undefined."

yes

The linux man page (glibc I suppose) has no such mention, only
disallows longjmp and the phrase I cited above. I think this
establishes an extension for POSIX on Linux systems.

But I have another reason for wanting that, future compatibility with
C threads. Programs that are written for C threads will not be aware
of such interdictions. Concretely in our case of my C thread v3 patch
a user can longjmp from a once-init-handler (written by her or him)
through pthread_once (in libc, for musl with pthread_cleanup_push) to
the thread start function (again user code) and then return from
there. (All of this seems to be allowed by POSIX)

I consider to not execute the cleanup handlers a little bit more
friendly than executing them.

Another possibility would be to split the behavior

 - abort for pthreads (this is nicer than executing the handlers or
   than silently ignore them)

 - ignore for C threads

Jens

-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Rich Felker]

On Tue, Aug 05, 2014 at 09:06:16PM +0200, Jens Gustedt wrote:
> Hello,
> 
> Am Dienstag, den 05.08.2014, 13:09 -0400 schrieb Rich Felker:
> > > Linux manpages are more explicit and state:
> > > 
> > >     Clean-up handlers are not called if the thread terminates by
> > >     performing a return from the thread start function.
> > > 
> > > This patch aligns musl to that behavior.
> > 
> > Could you clarify why this patch is necessary? I think such a return
> > is explicitly UB.
> > 
> > "The effect of the use of return, break, continue, and goto to
> > prematurely leave a code block described by a pair of
> > pthread_cleanup_push() and pthread_cleanup_pop() functions calls is
> > undefined."
> 
> yes
> 
> The linux man page (glibc I suppose) has no such mention, only
> disallows longjmp and the phrase I cited above. I think this
> establishes an extension for POSIX on Linux systems.

I think what's going on here is that glibc implements cancellation
cleanup as unwinding, so whatever happens to be the behavior that
results from unwinding is what you get. However, I would think that
would cause the cleanup handlers to get run when returning from the
start function, rather than not running, since it's actually the
compiler that calls them, and the compiler has no way of knowing the
start function is special.

musl explicitly does not use unwinding for cancellation, and does not
attempt to provide related glibc semantics which are all in the realm
of UB per POSIX.

> But I have another reason for wanting that, future compatibility with
> C threads. Programs that are written for C threads will not be aware
> of such interdictions. Concretely in our case of my C thread v3 patch
> a user can longjmp from a once-init-handler (written by her or him)
> through pthread_once (in libc, for musl with pthread_cleanup_push) to
> the thread start function (again user code) and then return from
> there. (All of this seems to be allowed by POSIX)

Such a longjmp is UB unless it's explicitly permitted by the standard
-- same as longjmp out of qsort. There's no guarantee to a function
called as a callback from the standard library that the calling
function does not have internal state which would be left in an
inconsistent state by longjmp'ing out.

If call_once is explicitly required to support longjmp, then a
separate implementation of call_once is needed for C11 without
cancellation support. However I doubt this usage is supported, since
the state of the once_flag would have to be specified for such
interrupted calls, and I see no such specification.

> I consider to not execute the cleanup handlers a little bit more
> friendly than executing them.

musl does not do either. It's simply UB. At the point where you're
claiming the cleanup handler is executed, the pointer being
dereferenced is to an object whose lifetime has ended, so it's not
even making a call to the cleanup handler but simply invoking UB.

> Another possibility would be to split the behavior
> 
>  - abort for pthreads (this is nicer than executing the handlers or
>    than silently ignore them)
> 
>  - ignore for C threads

There's no way it can happen for C threads unless they're actually
using pthread_cleanup_push, and the relevenat code that might be
returning is aware of POSIX and the reules for use of
pthread_cleanup_push.

Rich

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Rich Felker]

On Tue, Aug 05, 2014 at 03:41:17PM -0400, Rich Felker wrote:
> > But I have another reason for wanting that, future compatibility with
> > C threads. Programs that are written for C threads will not be aware
> > of such interdictions. Concretely in our case of my C thread v3 patch
> > a user can longjmp from a once-init-handler (written by her or him)
> > through pthread_once (in libc, for musl with pthread_cleanup_push) to
> > the thread start function (again user code) and then return from
> > there. (All of this seems to be allowed by POSIX)
> 
> Such a longjmp is UB unless it's explicitly permitted by the standard
> -- same as longjmp out of qsort. There's no guarantee to a function
> called as a callback from the standard library that the calling
> function does not have internal state which would be left in an
> inconsistent state by longjmp'ing out.

Actually, I can't find the text which supports this, which is odd
since it seems to be common knowledge that you can implement qsort
with algorithms that use allocation if it succeeds. The standard
_does_ contain such text for atexit and at_quick_exit, and I suppose
qsort is covered by the strict interface contract for the comparison
function (the "shall return" requirement cannot be met if the function
calls longjmp). The only other standard library function I'm aware of
which calls back to application code is call_once, so it's not clear
to me whether lack of consideration of longjmp for call_once is an
omission (likely) or an allowance for it to use longjmp (in which the
"exactly once" is probably under-specified, especially since call_once
does not synchronize until "completion of an effective call").

Rich

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 4483 bytes --]

Hello,

Am Dienstag, den 05.08.2014, 15:41 -0400 schrieb Rich Felker:
> On Tue, Aug 05, 2014 at 09:06:16PM +0200, Jens Gustedt wrote:
> > But I have another reason for wanting that, future compatibility with
> > C threads. Programs that are written for C threads will not be aware
> > of such interdictions. Concretely in our case of my C thread v3 patch
> > a user can longjmp from a once-init-handler (written by her or him)
> > through pthread_once (in libc, for musl with pthread_cleanup_push) to
> > the thread start function (again user code) and then return from
> > there. (All of this seems to be allowed by POSIX)
> 
> Such a longjmp is UB unless it's explicitly permitted by the standard
> -- same as longjmp out of qsort.

Which standard are you refering to? I don't find any mention of such a
thing in the C standard. In the contrary, I find a lot of contexts
where longjmp is explicitly forbidden, such as at_exit and
similar. bsearch, qsort and call_once are not among these, and there
is no general mention of longjmp and "callback" contexts.

(I just find your second message. I don't find the "shall return" for
the comparison function as binding to exclude longjmp, for bsearch
this might even be tempting. For call_once, I agree, it is probably
lack of consideration.)

> There's no guarantee to a function
> called as a callback from the standard library that the calling
> function does not have internal state which would be left in an
> inconsistent state by longjmp'ing out.

Don't get me wrong, it isn't a brilliant idea to do longjmp from such
callbacks, but I don't see that you have an argument here. The
definition of setjmp/longjmp clearly states which circumstance make it
UB to use them. For the rest, this *defines* the behavior for all
other contexts, including callbacks, unless explicitly forbidden.

> If call_once is explicitly required to support longjmp,

It is not explicitly forbidden and I think this is sufficient to allow
for it.

I don't know about the equivalent for the pthread tools, though.

> then a separate implementation of call_once is needed for C11

yes

> without cancellation support.

no, it needs cancellation support that doesn't use local objects, see
below.

> However I doubt this usage is supported, since
> the state of the once_flag would have to be specified for such
> interrupted calls, and I see no such specification.
> 
> > I consider to not execute the cleanup handlers a little bit more
> > friendly than executing them.
> 
> musl does not do either. It's simply UB. At the point where you're
> claiming the cleanup handler is executed, the pointer being
> dereferenced is to an object whose lifetime has ended, so it's not
> even making a call to the cleanup handler but simply invoking UB.

Aren't you playing a bit with words, here? Fact is, that the handler
might be called in real life. We are not obliged to be nasty when (and
if!) the user is invoking UB.

For the case of pthread_once or call_once, we could get away with it
by having the `struct __ptcb` static at that place. The caller of the
callback holds a lock at the point of call, so there would be no race
on such a static object.

I will prepare a patch for call_once to do so, just to be on the safe
side. This would incur 0 cost.

The same could be used for pthread_once.

> > Another possibility would be to split the behavior
> > 
> >  - abort for pthreads (this is nicer than executing the handlers or
> >    than silently ignore them)
> > 
> >  - ignore for C threads
> 
> There's no way it can happen for C threads unless they're actually
> using pthread_cleanup_push, and the relevenat code that might be
> returning is aware of POSIX and the reules for use of
> pthread_cleanup_push.

I don't think it is as simple as that. From C you might use a third
party library that on a specific platform uses pthread features under
the hood. E.g gcc's libatomic is such a candidate, they are using
pthread_mutex_t without telling you. OpenMP also comes in mind. And
who knows want the future will bring on such mixed platforms.

Jens

-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Rich Felker]

On Tue, Aug 05, 2014 at 11:05:41PM +0200, Jens Gustedt wrote:
> Hello,
> 
> Am Dienstag, den 05.08.2014, 15:41 -0400 schrieb Rich Felker:
> > On Tue, Aug 05, 2014 at 09:06:16PM +0200, Jens Gustedt wrote:
> > > But I have another reason for wanting that, future compatibility with
> > > C threads. Programs that are written for C threads will not be aware
> > > of such interdictions. Concretely in our case of my C thread v3 patch
> > > a user can longjmp from a once-init-handler (written by her or him)
> > > through pthread_once (in libc, for musl with pthread_cleanup_push) to
> > > the thread start function (again user code) and then return from
> > > there. (All of this seems to be allowed by POSIX)
> > 
> > Such a longjmp is UB unless it's explicitly permitted by the standard
> > -- same as longjmp out of qsort.
> 
> Which standard are you refering to? I don't find any mention of such a
> thing in the C standard. In the contrary, I find a lot of contexts
> where longjmp is explicitly forbidden, such as at_exit and
> similar. bsearch, qsort and call_once are not among these, and there
> is no general mention of longjmp and "callback" contexts.
> 
> (I just find your second message. I don't find the "shall return" for
> the comparison function as binding to exclude longjmp, for bsearch
> this might even be tempting. For call_once, I agree, it is probably
> lack of consideration.)

How is a "shall return" not binding? It does not say "if the function
returns, the return value shall be..."; it says "shall return". I
would say this means infinite loops in a comparison function are also
invalid, and a compiler doing optimization of qsort inline could
rightfully assume no infinite loops occur in the comparison function.

> > There's no guarantee to a function
> > called as a callback from the standard library that the calling
> > function does not have internal state which would be left in an
> > inconsistent state by longjmp'ing out.
> 
> Don't get me wrong, it isn't a brilliant idea to do longjmp from such
> callbacks, but I don't see that you have an argument here. The
> definition of setjmp/longjmp clearly states which circumstance make it
> UB to use them. For the rest, this *defines* the behavior for all
> other contexts, including callbacks, unless explicitly forbidden.

//

> 
> > If call_once is explicitly required to support longjmp,
> 
> It is not explicitly forbidden and I think this is sufficient to allow
> for it.

Actually I don't think it's possible to implement a call_once that
supports longjmp out without a longjmp which does unwinding, and I
don't think it's the intent of WG14 to require unwinding. The
fundamental problem is the lack of any way to synchronize "all
subsequent calls to the call_once function with the same value of
flag" when the code to update flag to indicate completion of the call
is skipped.

Do you see any solution to this problem? Perhaps a conforming
implementation can just deadlock all subsequent calls, since (1)
calling the function more than once conflicts with the "exactly once"
requirement, and (2) the completion of the first call, which
subsequent calls have to synchronize with, never happens.

> I don't know about the equivalent for the pthread tools, though.

I can't immediately find any information on the subject, so this may
call for a request for interpretation.

> > However I doubt this usage is supported, since
> > the state of the once_flag would have to be specified for such
> > interrupted calls, and I see no such specification.
> > 
> > > I consider to not execute the cleanup handlers a little bit more
> > > friendly than executing them.
> > 
> > musl does not do either. It's simply UB. At the point where you're
> > claiming the cleanup handler is executed, the pointer being
> > dereferenced is to an object whose lifetime has ended, so it's not
> > even making a call to the cleanup handler but simply invoking UB.
> 
> Aren't you playing a bit with words, here? Fact is, that the handler
> might be called in real life. We are not obliged to be nasty when (and
> if!) the user is invoking UB.

No, I'm not. The only way an attempt to make the call happens is if
you leave the block context of a pthread_cleanup_push illegally. It's
explicitly stated that doing so is UB, and there's no easy way to do
it accidentally:

- Except for longjmp, the statement that leaves the block illegally
  would be local to the function that called pthread_cleanup_push, so
  the invalidity of the code is immediately apparent, and happens in a
  context where the programmer knows pthread_cleanup_push is in
  effect.

- In the case of longjmp, it's already dangerous to longjmp across
  arbitrary code. Unless you know what code you're jumping over is
  doing or have a contract that it's valid to jump over it, you can't
  use longjmp.

> For the case of pthread_once or call_once, we could get away with it
> by having the `struct __ptcb` static at that place. The caller of the
> callback holds a lock at the point of call, so there would be no race
> on such a static object.
> 
> I will prepare a patch for call_once to do so, just to be on the safe
> side. This would incur 0 cost.

The cost is encoding knowledge of the cancellation implementation at
the point of the call, and synchronizing all calls to call_once with
respect to each other, even when they don't use the same once_flag
object.

call_once does not have specified behavior if the callback is
cancelled (because, as far as C11 is concerned, cancellation does not
even exist). Thus there is no need for a cleanup function at all. I
think we should wait to do anything unneccessary, ugly, and/or complex
here unless the next issue of POSIX specifies a behavior for this
case. And even then, only if longjmp is allowed, which is unlikely
(see my above analysis).

> The same could be used for pthread_once.

Above analysis about longjmp being impossible to support without
deadlock or unwinding in longjmp also applies to pthread_once. I
suspect, if longjmp is implicitly allowed, this is just an oversight
and one which will be corrected.

> > > Another possibility would be to split the behavior
> > > 
> > >  - abort for pthreads (this is nicer than executing the handlers or
> > >    than silently ignore them)
> > > 
> > >  - ignore for C threads
> > 
> > There's no way it can happen for C threads unless they're actually
> > using pthread_cleanup_push, and the relevenat code that might be
> > returning is aware of POSIX and the reules for use of
> > pthread_cleanup_push.
> 
> I don't think it is as simple as that. From C you might use a third
> party library that on a specific platform uses pthread features under
> the hood. E.g gcc's libatomic is such a candidate, they are using
> pthread_mutex_t without telling you. OpenMP also comes in mind. And
> who knows want the future will bring on such mixed platforms.

We're talking about a return statement in between the
pthread_cleanup_push and pthread_cleanup_pop calls in a particular
block context. Third-party library code does not magically insert such
a statement in your code. So I don't understand the case you have in
mind.

Rich

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 5551 bytes --]

Am Dienstag, den 05.08.2014, 17:48 -0400 schrieb Rich Felker:
> On Tue, Aug 05, 2014 at 11:05:41PM +0200, Jens Gustedt wrote:
> > It is not explicitly forbidden and I think this is sufficient to allow
> > for it.
> 
> Actually I don't think it's possible to implement a call_once that
> supports longjmp out without a longjmp which does unwinding,

simple longjmp, perhaps not, longjmp that is followed by thread
termination, yes. I just have done it, so it is possible :)

As I mentioned earlier, you can do this by having everything
statically allocated. I am using a struct for once_flag that has the
`int` for control, another `int` for waiters and a `struct __ptcb` for
the call to _pthread_cleanup_push. call_once is just a copy of
pthread_once, just using these fields of the once_flag, instead of the
local variables.

(Strictly spoken, once_flag is not required to be statically
allocated, as is the explicit requirement for pthread_once_t. Yet
another omission of C11, it is on my list for addition.)

> and I
> don't think it's the intent of WG14 to require unwinding. The
> fundamental problem is the lack of any way to synchronize "all
> subsequent calls to the call_once function with the same value of
> flag" when the code to update flag to indicate completion of the call
> is skipped.

sure

> Do you see any solution to this problem? Perhaps a conforming
> implementation can just deadlock all subsequent calls, since (1)
> calling the function more than once conflicts with the "exactly once"
> requirement, and (2) the completion of the first call, which
> subsequent calls have to synchronize with, never happens.

one part of solution is the "static" implementation as mentioned
above, followed by a fight to get the requirement for static
allocation and interdiction of longjmp and thrd_exit into the
specification of call_once.

> - In the case of longjmp, it's already dangerous to longjmp across
>   arbitrary code. Unless you know what code you're jumping over is
>   doing or have a contract that it's valid to jump over it, you can't
>   use longjmp.

sure, but just consider the nice tool-library that implements
try-catch sort of things with setjmp/longjmp.

> > For the case of pthread_once or call_once, we could get away with it
> > by having the `struct __ptcb` static at that place. The caller of the
> > callback holds a lock at the point of call, so there would be no race
> > on such a static object.
> > 
> > I will prepare a patch for call_once to do so, just to be on the safe
> > side. This would incur 0 cost.
> 
> The cost is encoding knowledge of the cancellation implementation at
> the point of the call, and synchronizing all calls to call_once with
> respect to each other, even when they don't use the same once_flag
> object.

No, I don't think so, and with what I explained above this is perhaps
a bit clearer. The only thing that this does is making all the state
of a particular call to call_once statically allocated.

(There is even less interaction between calls to call_once with
different objects, than we have for the current implementation of
pthread_once. That one uses a `static int waiters` that is shared
between all calls.)

> call_once does not have specified behavior if the callback is
> cancelled (because, as far as C11 is concerned, cancellation does not
> even exist). Thus there is no need for a cleanup function at all.

After digging into it I don't agree. In a mixed pthread/C thread
environment cancellation will eventually happen, so we need a cleanup
function to avoid deadlocks.

> I think we should wait to do anything unneccessary, ugly, and/or
> complex here unless the next issue of POSIX specifies a behavior for
> this case.

too late, done already, and it is not too ugly, either

> > The same could be used for pthread_once.
> 
> Above analysis about longjmp being impossible to support without
> deadlock or unwinding in longjmp also applies to pthread_once. I
> suspect, if longjmp is implicitly allowed, this is just an oversight
> and one which will be corrected.

right

also introducing the same idea as above would be an ABI change, so
let's not touch this

> We're talking about a return statement in between the
> pthread_cleanup_push and pthread_cleanup_pop calls in a particular
> block context. Third-party library code does not magically insert such
> a statement in your code. So I don't understand the case you have in
> mind.

No it is not only return, it also is longjmp. As mentioned above,
consider a pure C11 context where I use a tool box that implements
try-catch things by means of setjmp/longjmp.

Then I use another, independent library that uses callbacks, that can
be graphics, OpenMp, libc, whatever. And perhaps on some POSIX system
it uses pthread_cleanup_push under the hood.

It will be hard to ensure that both tools when used together on an
arbitrary platform that implements them will not collide with the
requirement not to use longjmp across a pthread_cleanup context.

(BTW, in the musl timer code I just detected a nasty usage of longjmp
from within a cleanup handler. That one is ugly :)

Jens

-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Rich Felker]

On Wed, Aug 06, 2014 at 01:19:58AM +0200, Jens Gustedt wrote:
> Am Dienstag, den 05.08.2014, 17:48 -0400 schrieb Rich Felker:
> > On Tue, Aug 05, 2014 at 11:05:41PM +0200, Jens Gustedt wrote:
> > > It is not explicitly forbidden and I think this is sufficient to allow
> > > for it.
> > 
> > Actually I don't think it's possible to implement a call_once that
> > supports longjmp out without a longjmp which does unwinding,
> 
> simple longjmp, perhaps not, longjmp that is followed by thread
> termination, yes. I just have done it, so it is possible :)
> 
> As I mentioned earlier, you can do this by having everything
> statically allocated. I am using a struct for once_flag that has the
> `int` for control, another `int` for waiters and a `struct __ptcb` for
> the call to _pthread_cleanup_push. call_once is just a copy of
> pthread_once, just using these fields of the once_flag, instead of the
> local variables.

This doesn't solve the problem. The problem is not the lifetime of the
cancellation buffer (which doesn't even need to exist for call_once),
but the fact that there's no way to track that the in-progress call
was "finished" by longjmp so that another one can be started.

BTW a related issue is that recursive calls to call_once are
unspecified: what happens if the init function passed to call_once
itself calls call_once with the same once_flag object? :)

I've reported the pthread_once issue here, and added a note on the
other related items (recursive calls and pthread_exit) that came up
later:

http://austingroupbugs.net/view.php?id=863

> (Strictly spoken, once_flag is not required to be statically
> allocated, as is the explicit requirement for pthread_once_t. Yet
> another omission of C11, it is on my list for addition.)

In principle, it's nice for a call_once like function not to require
static storage. It means you can use it inside dynamically allocated
objects for ctor-like purposes where the ctor isn't always needed and
can be run lazily on the first operation that needs it. Of course
call_once is useless for this since it doesn't allow passing an
argument to the function. So I have no objection to requesting that
C11 be amended to require static storage for once_flag.

> > and I
> > don't think it's the intent of WG14 to require unwinding. The
> > fundamental problem is the lack of any way to synchronize "all
> > subsequent calls to the call_once function with the same value of
> > flag" when the code to update flag to indicate completion of the call
> > is skipped.
> 
> sure

So I don't see how you would solve this problem without just banning
use of longjmp from call_once.

> > Do you see any solution to this problem? Perhaps a conforming
> > implementation can just deadlock all subsequent calls, since (1)
> > calling the function more than once conflicts with the "exactly once"
> > requirement, and (2) the completion of the first call, which
> > subsequent calls have to synchronize with, never happens.
> 
> one part of solution is the "static" implementation as mentioned
> above,

I don't see how this fixes it. The cancellation buffer is not the
issue that matters. The issue is that what to do isn't even specified,
and unless the "what to do" is just deadlock, there's no good way to
detect that the once_flag object was 'released' by a longjmp to allow
it to be taken again.

> followed by a fight to get the requirement for static
> allocation and interdiction of longjmp and thrd_exit into the
> specification of call_once.

I don't think it will be much of a fight. BTW while you're at it,
there might be a number of additional issues -- like what happens if
atexit handlers call thrd_exit, what happens if tss dtors longjmp,
etc. As far as I can tell, none of this is properly specified and IMO
it should all be explicit UB.

> > - In the case of longjmp, it's already dangerous to longjmp across
> >   arbitrary code. Unless you know what code you're jumping over is
> >   doing or have a contract that it's valid to jump over it, you can't
> >   use longjmp.
> 
> sure, but just consider the nice tool-library that implements
> try-catch sort of things with setjmp/longjmp.

It's not possible to mix this with cancellation (or with C++
exceptions, for that matter). But as long as neither "unwinds" across
the other, you're fine. This is a restriction you need to follow and
be aware of anyway. It's not something we can "work around" except by
using the same underlying implementation for all three, and that's
undesirable for various reasons (one of which is that people should
not be relying on it).

Even if you did want to support this, making it work is a lot harder
than allowing longjmp out of pthread_once/call_once. The latter are
special cases where there's at most one cancellation cleanup layer and
it's implementation-internal. The general case has arbitrary
cancellation cleanup buffers at different call frame contexts, and
they're most certainly not static.

> > > For the case of pthread_once or call_once, we could get away with it
> > > by having the `struct __ptcb` static at that place. The caller of the
> > > callback holds a lock at the point of call, so there would be no race
> > > on such a static object.
> > > 
> > > I will prepare a patch for call_once to do so, just to be on the safe
> > > side. This would incur 0 cost.
> > 
> > The cost is encoding knowledge of the cancellation implementation at
> > the point of the call, and synchronizing all calls to call_once with
> > respect to each other, even when they don't use the same once_flag
> > object.
> 
> No, I don't think so, and with what I explained above this is perhaps
> a bit clearer. The only thing that this does is making all the state
> of a particular call to call_once statically allocated.

OK, I misunderstood your design.

> (There is even less interaction between calls to call_once with
> different objects, than we have for the current implementation of
> pthread_once. That one uses a `static int waiters` that is shared
> between all calls.)

Yes, that's an unfortunate issue which I should fix. But making the
once_flag a big structure rather than a single int (which will surely
disagree with the glibc ABI) is not the solution. The solution is just
to have a potential-waiters flag in the single int, rather than a
waiter count. The incidence of contention is so low that it doesn't
matter if it results in a rare excess syscall, especially since once
calls are once-per-program-invocation.

> > call_once does not have specified behavior if the callback is
> > cancelled (because, as far as C11 is concerned, cancellation does not
> > even exist). Thus there is no need for a cleanup function at all.
> 
> After digging into it I don't agree. In a mixed pthread/C thread
> environment cancellation will eventually happen, so we need a cleanup
> function to avoid deadlocks.

No it won't. Cancellation is unusable with almost all existing code
(and almost all new code). To use cancellation, the target thread must
be running code which was written to be aware of the possibility of
being cancelled. Thus, it's not going to "just happen".

I have an in-progress proposal for an alternate cancellation mode
where the first cancellation point called with cancellation pending
would not act on it, but instead fail with ECANCELED. Setting this
mode would allow calling arbitrary third-party library code from
threads that might be subject to cancellation, and it would actually
behave as desired if the callee properly checks for errors and passes
error status back up to the caller. Unfortunately there are enough
details that I haven't worked out (not in the implementation, but in
how some special cases should behave) that I haven't implemented it or
proposed it yet.

> > I think we should wait to do anything unneccessary, ugly, and/or
> > complex here unless the next issue of POSIX specifies a behavior for
> > this case.
> 
> too late, done already, and it is not too ugly, either

What's already done?

> > > The same could be used for pthread_once.
> > 
> > Above analysis about longjmp being impossible to support without
> > deadlock or unwinding in longjmp also applies to pthread_once. I
> > suspect, if longjmp is implicitly allowed, this is just an oversight
> > and one which will be corrected.
> 
> right
> 
> also introducing the same idea as above would be an ABI change, so
> let's not touch this

Indeed.

> > We're talking about a return statement in between the
> > pthread_cleanup_push and pthread_cleanup_pop calls in a particular
> > block context. Third-party library code does not magically insert such
> > a statement in your code. So I don't understand the case you have in
> > mind.
> 
> No it is not only return, it also is longjmp. As mentioned above,
> consider a pure C11 context where I use a tool box that implements
> try-catch things by means of setjmp/longjmp.

This is UB and it won't be supported. This decision was made a long
time ago.

> Then I use another, independent library that uses callbacks, that can
> be graphics, OpenMp, libc, whatever. And perhaps on some POSIX system
> it uses pthread_cleanup_push under the hood.
> 
> It will be hard to ensure that both tools when used together on an
> arbitrary platform that implements them will not collide with the
> requirement not to use longjmp across a pthread_cleanup context.

No it's not. Only if one is calling to the other which calls back to
the first, and they're mixing their unwinding mechanisms across these
boundaries, can any such issue happen. And this is explicitly UB and
explicitly nonsense.

> (BTW, in the musl timer code I just detected a nasty usage of longjmp
> from within a cleanup handler. That one is ugly :)

Yes, but this is implementation-internal and used correctly based on
the properties of the cleanup implementation. It's needed to reuse the
same thread (which is needed because we can't necessarily make a new
one.)

Rich

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 7385 bytes --]

Am Dienstag, den 05.08.2014, 22:02 -0400 schrieb Rich Felker:
> > As I mentioned earlier, you can do this by having everything
> > statically allocated. I am using a struct for once_flag that has the
> > `int` for control, another `int` for waiters and a `struct __ptcb` for
> > the call to _pthread_cleanup_push. call_once is just a copy of
> > pthread_once, just using these fields of the once_flag, instead of the
> > local variables.
> 
> This doesn't solve the problem. The problem is not the lifetime of the
> cancellation buffer (which doesn't even need to exist for call_once),
> but the fact that there's no way to track that the in-progress call
> was "finished" by longjmp so that another one can be started.

I don't know if this "solves" one of the problems related to that, but
it makes the code more robust, would enable us to give a decent
diagnostic before crashing and would probably ease debugging of such
crappy code.

> BTW a related issue is that recursive calls to call_once are
> unspecified: what happens if the init function passed to call_once
> itself calls call_once with the same once_flag object? :)
> 
> I've reported the pthread_once issue here, and added a note on the
> other related items (recursive calls and pthread_exit) that came up
> later:
> 
> http://austingroupbugs.net/view.php?id=863

Nice summary.

(I'd wish, WG14 had such a "bug tracker")

> > (Strictly spoken, once_flag is not required to be statically
> > allocated, as is the explicit requirement for pthread_once_t. Yet
> > another omission of C11, it is on my list for addition.)
> 
> In principle, it's nice for a call_once like function not to require
> static storage. It means you can use it inside dynamically allocated
> objects for ctor-like purposes where the ctor isn't always needed and
> can be run lazily on the first operation that needs it. Of course
> call_once is useless for this since it doesn't allow passing an
> argument to the function. So I have no objection to requesting that
> C11 be amended to require static storage for once_flag.

Yes such a dynamic initialization tool would be nice (P99 has it) and
I agree that once_flag (or pthread_once) isn't it.

I'd also just go for the "static" because POSIX has this, and I don't
think that it is a good idea to get the semantics of the different
control structures too much out of sync between the two standards.

> > followed by a fight to get the requirement for static
> > allocation and interdiction of longjmp and thrd_exit into the
> > specification of call_once.
> 
> I don't think it will be much of a fight.

sometimes I am very much surprised what proposals are vetoed

a valid initial state of statically allocated atomics or mutexes is
one of those that I already tried to convince people, meeting a
surprisingly strong resistance

> BTW while you're at it,
> there might be a number of additional issues -- like what happens if
> atexit handlers call thrd_exit, what happens if tss dtors longjmp,
> etc. As far as I can tell, none of this is properly specified and IMO
> it should all be explicit UB.

Yes.

I just also added recursive calls to call_once on the same object to
my list.

> > > - In the case of longjmp, it's already dangerous to longjmp across
> > >   arbitrary code. Unless you know what code you're jumping over is
> > >   doing or have a contract that it's valid to jump over it, you can't
> > >   use longjmp.
> > 
> > sure, but just consider the nice tool-library that implements
> > try-catch sort of things with setjmp/longjmp.
> 
> It's not possible to mix this with cancellation (or with C++
> exceptions, for that matter). But as long as neither "unwinds" across
> the other, you're fine. This is a restriction you need to follow and
> be aware of anyway. It's not something we can "work around" except by
> using the same underlying implementation for all three, and that's
> undesirable for various reasons (one of which is that people should
> not be relying on it).
> 
> Even if you did want to support this, making it work is a lot harder
> than allowing longjmp out of pthread_once/call_once. The latter are
> special cases where there's at most one cancellation cleanup layer and
> it's implementation-internal. The general case has arbitrary
> cancellation cleanup buffers at different call frame contexts, and
> they're most certainly not static.

Perhaps I wouldn't like to support this, but make the library more
robust against such issues as said above. Give diagnostics ("cleanup
handler stack not empty after thread termination") and ease debugging.

If, as we seem both to intend, all this ends up to be clearly UB,
nothing prevents us from being nice if this happens. What do you think
of my initial patch (which is not very intrusive), but instead of
clearing `cancelbuf` to call abort?

> > (There is even less interaction between calls to call_once with
> > different objects, than we have for the current implementation of
> > pthread_once. That one uses a `static int waiters` that is shared
> > between all calls.)
> 
> Yes, that's an unfortunate issue which I should fix. But making the
> once_flag a big structure rather than a single int (which will surely
> disagree with the glibc ABI) is not the solution. The solution is just
> to have a potential-waiters flag in the single int, rather than a
> waiter count. The incidence of contention is so low that it doesn't
> matter if it results in a rare excess syscall, especially since once
> calls are once-per-program-invocation.

Sure, as I said changing the ABI for pthread_once is out of the
question.

Yes, I agree that this is a minor "problem", if at all.

You could even have a counter in there, the state only needs 2 bit,
the rest could be used as a wait counter.

One idea would be to have that int as a counter, and to have special
values of the counter reflect the state. 0 would be unitialized, -1
completely initialized, all other would be the number of waiters +1.

> > > call_once does not have specified behavior if the callback is
> > > cancelled (because, as far as C11 is concerned, cancellation does not
> > > even exist). Thus there is no need for a cleanup function at all.
> > 
> > After digging into it I don't agree. In a mixed pthread/C thread
> > environment cancellation will eventually happen, so we need a cleanup
> > function to avoid deadlocks.
> 
> No it won't. Cancellation is unusable with almost all existing code
> (and almost all new code). To use cancellation, the target thread must
> be running code which was written to be aware of the possibility of
> being cancelled. Thus, it's not going to "just happen".

With "will happen" I mean that code will just do it, perhaps by some
tool implemented with pthreads shooting at a C thread.

> > too late, done already, and it is not too ugly, either
> 
> What's already done?

the call_once implementation with statically allocated state

Jens

-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Rich Felker]

On Wed, Aug 06, 2014 at 09:15:26AM +0200, Jens Gustedt wrote:
> > > > - In the case of longjmp, it's already dangerous to longjmp across
> > > >   arbitrary code. Unless you know what code you're jumping over is
> > > >   doing or have a contract that it's valid to jump over it, you can't
> > > >   use longjmp.
> > > 
> > > sure, but just consider the nice tool-library that implements
> > > try-catch sort of things with setjmp/longjmp.
> > 
> > It's not possible to mix this with cancellation (or with C++
> > exceptions, for that matter). But as long as neither "unwinds" across
> > the other, you're fine. This is a restriction you need to follow and
> > be aware of anyway. It's not something we can "work around" except by
> > using the same underlying implementation for all three, and that's
> > undesirable for various reasons (one of which is that people should
> > not be relying on it).
> > 
> > Even if you did want to support this, making it work is a lot harder
> > than allowing longjmp out of pthread_once/call_once. The latter are
> > special cases where there's at most one cancellation cleanup layer and
> > it's implementation-internal. The general case has arbitrary
> > cancellation cleanup buffers at different call frame contexts, and
> > they're most certainly not static.
> 
> Perhaps I wouldn't like to support this, but make the library more
> robust against such issues as said above. Give diagnostics ("cleanup
> handler stack not empty after thread termination") and ease debugging.
> 
> If, as we seem both to intend, all this ends up to be clearly UB,
> nothing prevents us from being nice if this happens. What do you think
> of my initial patch (which is not very intrusive), but instead of
> clearing `cancelbuf` to call abort?

Your initial patch only covered the case where the illegal return is
from the start function itself, which I think is probably the least
likely to happen in practice. It doesn't cover the case where it
happens from other functions. If this is deemed useful to catch and
translate into a predictable crash, I would prefer a check for the
cancellation buffer pointer against the stack pointer, crashing (via
a_crash() like other code that does light checks for UB such as the
code in malloc) if the cancellation buffer is below the stack pointer.
However some consideration is needed (like: do we want to preclude
cancellation from crossing signal handler boundaries when it's done
"safely", e.g. by pushing a cleanup handler before unblocking signals,
when the signal runs on an alternate signal stack?) so I'm a bit
hesitant to add such checks that might not always be valid. Of course
split-stack would also invalidate this strategy, but I think
split-stack is already hopelessly broken anyway (there's a good past
mailing list thread on the topic of split-stack and why it's a
solution in search of a problem :).

BTW one "safety" we currently have is that pthread_cleanup_pop does
not just "pop one cleanup context"; it resets the context to the one
that was in effect at the time the corresponding push was called,
potentially popping multiple contexts if one or more pop was skipped
via illegal returns or similar. In principle a check-and-crash could
be added asserting that self->cancelbuf == cb before doing this, but
I'm mildly hesitant to add conditionals to something that should be a
fast-path.

> > > (There is even less interaction between calls to call_once with
> > > different objects, than we have for the current implementation of
> > > pthread_once. That one uses a `static int waiters` that is shared
> > > between all calls.)
> > 
> > Yes, that's an unfortunate issue which I should fix. But making the
> > once_flag a big structure rather than a single int (which will surely
> > disagree with the glibc ABI) is not the solution. The solution is just
> > to have a potential-waiters flag in the single int, rather than a
> > waiter count. The incidence of contention is so low that it doesn't
> > matter if it results in a rare excess syscall, especially since once
> > calls are once-per-program-invocation.
> 
> Sure, as I said changing the ABI for pthread_once is out of the
> question.
> 
> Yes, I agree that this is a minor "problem", if at all.
> 
> You could even have a counter in there, the state only needs 2 bit,
> the rest could be used as a wait counter.
> 
> One idea would be to have that int as a counter, and to have special
> values of the counter reflect the state. 0 would be unitialized, -1
> completely initialized, all other would be the number of waiters +1.

Yes, that's probably a slightly better design. As long as our target
is just a basically-linux-compatible system, we have at least 2 bits
free anyway (tids are limited below 1<<30 since the upper bits are
used for special purposes in the robust futex API) but of course a
design that doesn't encode this knowledge is more general and cleaner.

> > > > call_once does not have specified behavior if the callback is
> > > > cancelled (because, as far as C11 is concerned, cancellation does not
> > > > even exist). Thus there is no need for a cleanup function at all.
> > > 
> > > After digging into it I don't agree. In a mixed pthread/C thread
> > > environment cancellation will eventually happen, so we need a cleanup
> > > function to avoid deadlocks.
> > 
> > No it won't. Cancellation is unusable with almost all existing code
> > (and almost all new code). To use cancellation, the target thread must
> > be running code which was written to be aware of the possibility of
> > being cancelled. Thus, it's not going to "just happen".
> 
> With "will happen" I mean that code will just do it, perhaps by some
> tool implemented with pthreads shooting at a C thread.

Yes. But my point is that crashing in libc code is probably the least
of your worries here. It's more likely that the crash (or at least
deadlock) occurs in application code from cancelling a thread while
it's in an inconsistent state, holds locks, etc. without a chance to
do cleanup (since the target thread was not designed for
cancellation).

One "safe" approach might be for call_once to simply block
cancellation for the duration of the call. In fact C11 threads could
even start with cancellation blocked, unless of course POSIX mandates
otherwise in the next issue. This should really all be a topic for
discussion with the Austin Group for how POSIX should be aligned with
C11 threads, though, and I'm hesitant to make any public behaviors for
musl that look like API guarantees (rather than "this is the way it
works for now, but don't depend on it, since it's going to change when
the next issue of POSIX is published") at this time.

Rich

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 2705 bytes --]

Am Mittwoch, den 06.08.2014, 05:35 -0400 schrieb Rich Felker:
> BTW one "safety" we currently have is that pthread_cleanup_pop does
> not just "pop one cleanup context"; it resets the context to the one
> that was in effect at the time the corresponding push was called,
> potentially popping multiple contexts if one or more pop was skipped
> via illegal returns or similar. In principle a check-and-crash could
> be added asserting that self->cancelbuf == cb before doing this, but
> I'm mildly hesitant to add conditionals to something that should be a
> fast-path.

You would have my +1 for that. Everyone should be aware that cleanup
push/pop comes with a cost and one conditional jump really doesn't add
much to the cost.

> Yes. But my point is that crashing in libc code is probably the least
> of your worries here. It's more likely that the crash (or at least
> deadlock) occurs in application code from cancelling a thread while
> it's in an inconsistent state, holds locks, etc. without a chance to
> do cleanup (since the target thread was not designed for
> cancellation).

I am not sure. With C11 threads as they are designed now, we will have
more dynamic initialization than with pthreads. This is for the simple
reason that there is no guaranteed static initialization for mutexes
or conditions. Either doing the initialization in `main` before
starting threads or once_call is the only (official) way to
go. Debugging an application that crashes during such a dynamic
startup (or the corresponding shutdown) can be really nasty.

> One "safe" approach might be for call_once to simply block
> cancellation for the duration of the call. In fact C11 threads could
> even start with cancellation blocked, unless of course POSIX mandates
> otherwise in the next issue.

right, good idea, I'll give it a thought

> This should really all be a topic for discussion with the Austin
> Group for how POSIX should be aligned with C11 threads, though, and
> I'm hesitant to make any public behaviors for musl that look like
> API guarantees (rather than "this is the way it works for now, but
> don't depend on it, since it's going to change when the next issue
> of POSIX is published") at this time.

Yes, this shouldn't be guaranteed unless POSIX mandates it. But it
could documented as an implementation and version dependent choice.

Jens

-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: PATCH: don't call cleanup handlers after a regular return from the thread start function

[Rich Felker]

On Wed, Aug 06, 2014 at 12:00:47PM +0200, Jens Gustedt wrote:
> Am Mittwoch, den 06.08.2014, 05:35 -0400 schrieb Rich Felker:
> > BTW one "safety" we currently have is that pthread_cleanup_pop does
> > not just "pop one cleanup context"; it resets the context to the one
> > that was in effect at the time the corresponding push was called,
> > potentially popping multiple contexts if one or more pop was skipped
> > via illegal returns or similar. In principle a check-and-crash could
> > be added asserting that self->cancelbuf == cb before doing this, but
> > I'm mildly hesitant to add conditionals to something that should be a
> > fast-path.
> 
> You would have my +1 for that. Everyone should be aware that cleanup
> push/pop comes with a cost and one conditional jump really doesn't add
> much to the cost.

Indeed. And it should be a 100% predictable jump. My main concern then
is whether we can ensure that it doesn't have false-positives (crashes
for valid-but-ugly usage).

> > One "safe" approach might be for call_once to simply block
> > cancellation for the duration of the call. In fact C11 threads could
> > even start with cancellation blocked, unless of course POSIX mandates
> > otherwise in the next issue.
> 
> right, good idea, I'll give it a thought

The latter probably doesn't help, since obviously the main thread
can't start with cancellation disabled, and call_once is likely to be
called from a thread started with pthread_create in applications where
the C11 thread code is just in a thread-safe library that doesn't
itself make the threads that are doing the calling.

On the other hand, having call_once block cancellation should
eliminate the problem, but there's a chance POSIX would not allow that
behavior.

We should really make a summary of the issues that have come up in
this thread to deliver for consideration when the Austin Group works
on aligning POSIX with C11. There's a much better chance that they'll
avoid specifying things that would be problematic if there's
documented discussion of the issues that arise.

Rich