Re: [PATCH] Secure newlib stdio functions against thread cancellation (was Re: newlib stdio and thread cancellation)

[Corinna Vinschen <vinschen at redhat dot com>]

On May 29 15:18, Jeff Johnston wrote:
> On 05/29/2012 01:25 PM, Corinna Vinschen wrote:
> >Below is a patch to fix the problem.  I had a look into the NetBSD code
> >and it turned out that NetBSD does not push and pop thread cleanup
> >handlers, but rather it recursively disables thread cancellation for the
> >duration of the stdio function.
> >
> >The below patch accomplishes thread cancellation safety the same way.  I
> >defined new macros to encapsulate the _flockfile/_funlockfile calls, as
> >well as the __sfp_lock_acquire/__sfp_lock_release calls, since they
> >suffer the same problem.  Then I replaced the calls throughout without
> >changing the structure of the code.
> >
> >The macros are defined differently depending on the values of
> >__SINGLE_THREAD__ and _POSIX_THREADS.  Only if both are defined, the
> >pthread cancel state is changed, otherwise the macros are defined to
> >just call the usual lock/unlock functions.
> >
> >The only functions left as an excercise are the __fp_lock_all and
> >__fp_unlock_all functions.  In case of using them, the *caller* has to
> >make sure that it handles thread cancellation correctly.  However,
> >__fp_lock_all/__fp_unlock_all are not used in newlib anyway, and the
> >only consumer I know of is Cygwin's fork implementation.
> >
> >
> >Jeff, is that patch ok to apply?
> >
> 
> Looks fine.  The comments for the macros could add a brief clause
> that the start macro and end macros add starting and closing braces
> so must appear at the same nesting level.  The exit macros are for
> deeper nesting levels such as failing clauses and require an earlier
> start macro be used.

Thanks, applied.

I added a longish comment to libc/stdio/local.h to explain, I hope it
sufficiently explains the usage of the macros:

/* The following macros are supposed to replace calls to _flockfile/_funlockfile
   and __sfp_lock_acquire/__sfp_lock_release.  In case of multi-threaded
   environments using pthreads, it's not sufficient to lock the stdio functions
   against concurrent threads accessing the same data, the locking must also be
   secured against thread cancellation.

   The below macros have to be used in pairs.  The _newlib_XXX_start macro
   starts with a opening curly brace, the _newlib_XXX_end macro ends with a
   closing curly brace, so the start macro and the end macro mark the code
   start and end of a critical section.  In case the code leaves the critical
   section before reaching the end of the critical section's code end, use
   the appropriate _newlib_XXX_exit macro. */

I also found that the macros don't do their job well if fp and sfp_lock
locking is used interlocked.  That's the case in three files, fclose.c,
freopen.c, and freopen64.c.  In these three files, I coded disabling and
re-enabling thread cancellation explicitely.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat