Re: [PATCH] Fix potential reent issue

[Jeff Johnston <jjohnstn at redhat dot com>]

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.


Index: stdio.h
===================================================================
RCS file: /cvs/src/src/newlib/libc/include/stdio.h,v
retrieving revision 1.63
diff -p -r1.63 stdio.h
*** stdio.h	1 Nov 2012 11:51:11 -0000	1.63
--- stdio.h	24 Jun 2013 18:49:43 -0000
*************** typedef _fpos64_t fpos64_t;
*** 139,153 ****
  
  #define	TMP_MAX		26
  
- #ifndef _REENT_ONLY
  #define	stdin	(_REENT->_stdin)
  #define	stdout	(_REENT->_stdout)
  #define	stderr	(_REENT->_stderr)
- #else /* _REENT_ONLY */
- #define	stdin	(_impure_ptr->_stdin)
- #define	stdout	(_impure_ptr->_stdout)
- #define	stderr	(_impure_ptr->_stderr)
- #endif /* _REENT_ONLY */
  
  #define _stdin_r(x)	((x)->_stdin)
  #define _stdout_r(x)	((x)->_stdout)
--- 139,147 ----
Index: wchar.h
===================================================================
RCS file: /cvs/src/src/newlib/libc/include/wchar.h,v
retrieving revision 1.30
diff -p -r1.30 wchar.h
*** wchar.h	1 Nov 2012 11:51:11 -0000	1.30
--- wchar.h	24 Jun 2013 18:49:43 -0000
*************** int	_EXFUN(_wscanf_r, (struct _reent *, 
*** 179,191 ****
  
  #define getwc(fp)	fgetwc(fp)
  #define putwc(wc,fp)	fputwc((wc), (fp))
- #ifndef _REENT_ONLY
  #define getwchar()	fgetwc(_REENT->_stdin)
  #define putwchar(wc)	fputwc((wc), _REENT->_stdout)
- #else
- #define getwchar()	fgetwc(_impure_ptr->_stdin)
- #define putwchar(wc)	fputwc((wc), _impure_ptr->_stdout)
- #endif
  
  _END_STD_C
  
--- 179,186 ----
Index: sys/reent.h
===================================================================
RCS file: /cvs/src/src/newlib/libc/include/sys/reent.h,v
retrieving revision 1.53
diff -p -r1.53 reent.h
*** sys/reent.h	8 May 2013 23:13:51 -0000	1.53
--- sys/reent.h	24 Jun 2013 18:49:43 -0000
*************** void _reclaim_reent _PARAMS ((struct _re
*** 797,804 ****
  
  /* #define _REENT_ONLY define this to get only reentrant routines */
  
- #ifndef _REENT_ONLY
- 
  #if defined(__DYNAMIC_REENT__) && !defined(__SINGLE_THREAD__)
  #ifndef __getreent
    struct _reent * _EXFUN(__getreent, (void));
--- 797,802 ----
*************** void _reclaim_reent _PARAMS ((struct _re
*** 808,815 ****
  # define _REENT _impure_ptr
  #endif /* __SINGLE_THREAD__ || !__DYNAMIC_REENT__ */
  
- #endif /* !_REENT_ONLY */
- 
  #define _GLOBAL_REENT _global_impure_ptr
  
  #ifdef _REENT_GLOBAL_ATEXIT
--- 806,811 ----