Re: [PATCH] Fix potential reent issue

[Jeff Johnston <jjohnstn at redhat dot com>]

Patch has been checked in.

-- Jeff J.

----- Original Message -----
From: "Jeff Johnston" <jjohnstn@redhat.com>
To: newlib@sourceware.org
Sent: Monday, June 24, 2013 2:50:54 PM
Subject: Re: [PATCH] Fix potential reent issue

On 06/24/2013 05:37 AM, Corinna Vinschen wrote:
> On Jun 23 22:13, Freddie Chopin wrote:
>> W dniu 2013-06-23 21:40, Federico Terraneo pisze:
>>> On 06/23/2013 07:53 PM, Freddie Chopin wrote:
>>>> Hi!
>>>>
>>>> Wouldn't it be cleaner/simpler to use "_REENT" symbol (which is
>>>> either "_impure_ptr" or "__getreent()") in place of faulty uses of
>>>> _impure_ptr?
>>>>
>>>
>>> The problem is that when _REENT_ONLY is defined, _REENT does not get
>>> defined (look at sys/reent.h). I think it's this fact that caused the
>>> faulty use of _impure_ptr, but if the definition of _REENT is enclosed
>>> in an #ifndef _REENT_ONLY, there's probably a reason.
>>
>> Yes, you're right...
>>
>> Looking at repo history it seems that the _impure_ptr solution was
>> older, then - 11 years ago - the one using _REENT was added, but the
>> old variant remained unchanged for years.
>>
>> It seems to me that the "right" solution for _REENT_ONLY build would
>> be to undefine stdin/stdout/stderr - delete the offending lines
>> using _impure_ptr...
>>
>> If that solution is not feasible then your approach is probably the
>> best one.
>>
>> BTW - I've CCed the list, you probably forgot about that.
>
> I leave this problem for Jeff.
>

Let me first state that historically the _REENT_ONLY solution was 
initially added long ago for those who wanted to support multi-threading 
and thus had to keep track of separate reentrancy pointers.  The macro 
helped find usage of non _r functions.  I do not believe the std stream 
macros were ever restricted as part of this.  You either defaulted to 
the global reent struct or used the _r versions.

The addition of _DYNAMIC_REENT obsoleted the need for _REENT_ONLY 
because the __getreent() function would take care of finding the right 
reentrancy structure for all calls and the code did not need to use _r 
functions at all.

I do not think it is meaningful for anyone to use both macros at the 
same time.  If you choose to use _REENT_ONLY as well, then you are 
unnecessarily forcing the user to recode their application to only work 
under newlib.

Undefining the std stream macros now would be API removal that could 
break existing code and for no real good reason.

Regarding your patch, a better solution IMO would be to remove the 
_REENT_ONLY check in sys/reent.h so that the _REENT macro was defined 
and thus the std streams could always be defined in terms of _REENT.  It 
ends up the same and makes the header files cleaner.

I have attached the patch.

-- Jeff J.