Re: [PATCH] Fix problem with file locking used before initialised

[Jeff Johnston <jjohnstn at redhat dot com>]

Antony KING wrote:
> I have had a bit of a think on this and I don't think the addition of 
> this lock to __sinit is required since __sinit is called with a task 
> specific reent structure and does not reference any global state (other 
> than that in the reent structure).
>
> __sinit should therefore be inherently thread safe without the addition 
> of locks unless your application/RTOS is using this API in such a way 
> that it is possible for one task to initialise the stdio data of another 
> task's reent structure (and therefore have a potential race condition). 
> However I believe this scenario is not the model expected, nor 
> supported, by newlib (correct me if I am wrong in my understanding :-); 
> the expected model, I believe, is that a reent structure should only be 
> changed by its owner task.

You're right, a thinko on my part; I was also thinking of an enhancement I have 
been contemplating which will allow sharing of the std streams via _GLOBAL_REENT 
and having an alternate version of CHECK_INIT that ignores the input reentrant 
structure and passes _GLOBAL_REENT to _sinit.

> The only place in newlib where this seems to happen is __sfp (also in 
> findfp.c) where it checks the stdio initialisation of the global reent 
> structure (_GLOBAL_REENT). However this is safe since __sfp is already 
> taking a lock before the initialisation.
>
> Also, I note that I missed libgloss/arm/syscalls.c when I made the 
> changes to newlib/libc/sys/arm/syscalls.c in my recent patch and 
> therefore you may want to apply the same changes to this file (the files 
> should be identical).

Actually, I had done this but forgot to check it in.  Patch checked in.

Thanks,

-- Jeff J.

> Cheers,
>
> Antony.
>
> Jeff Johnston wrote:
>
> > Patch applied.  I noticed a flaw in that __sinit was unlocked so you 
> > could conceivably have two threads that called it in a race 
> > condition.  I added a lock for it and added a check once you acquire 
> > the lock that __sdidinit wasn't already set.
> >
> > Thanks,
> >
> > -- Jeff J.