This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: ping [PATCH]: Allow _rename_r to be re-classified as a system call, [PATCH]: Remove "is closed check" in fclose.c and fwalk.c


Jeff Johnston wrote:
> Antony KING wrote:
>> Hi,
>>
>> Its been a while :-) since I posted these patches so I thought I might
>> try again.
>>
>> The original submissions can be found on the newlib mailing list at the
>> following URLs:
>>
>> http://www.sourceware.org/ml/newlib/2006/msg00872.html
> 
> For the rename problem, I propose an alternate patch.  This treats it
> like fcntl.  The existing flags HAVE_RENAME and
> REENTRANT_SYSCALLS_PROVIDED flags are used.  If you say HAVE_RENAME and
> REENTRANT_SYSCALLS_PROVIDED, then you are assumed to provide _rename_r
> like the other syscalls.  This should not cause any problems for
> existing platforms.

This is fine. It is a better and more consistent solution than my
original suggestion.

>> http://www.sourceware.org/ml/newlib/2006/msg00871.html
>>
> 
> Please explain the scenario further.  I don't understand why locking
> and, more importantly, subsequently unlocking the file causes a problem
> for you.

After looking at my original description I see that you are right, it is
inadequate in describing the problems :-).

The root of the changes is that when a FILE is closed its associated
lock object is also "closed" (in fclose.c). Depending on the lock object
implementation this may leave the lock object in an undefined state and
therefore unusable. Its state will only be guaranteed when the FILE is
subsequently reused and opened, whereupon the lock object will be
initialised (by _sfp() in findfp.c). Of course if the lock object is
never opened then its lock object will also be in an undefined state.

As a consequence the code I removed in fclose(), to handle an already
closed file, seemed to be of dubious value. If the FILE pointer is
"random" memory then "all bets are off" anyway; if it is actually a FILE
object then taking a lock on the closed FILE is also risky (since it may
be in an undefined state), so unlocking it again is moot - we will
probably have already damaged our system. Of course if the lock object
implementation tolerates this then OK, the code could be left but it is
a grey area which is implementation dependent.

The changes in fwalk.c should be easier to explain :-). Consider the
case when the fwalk functions are passed fclose(). Without my change the
sequence of actions on the lock object will be:

* __lock_acquire_recursive (by fwalk)
* __lock_acquire_recursive (by fclose)
* __lock_release_recursive (by fclose)
* __lock_close_recursive (by fclose)    <== lock still acquired here
* __lock_release_recursive (by fwalk)

As you can see the lock is "closed" while it may still be acquired (e.g.
a mutex which expects the number of unlocks to match the number of locks
before the mutex is released). Depending on the OS implementation of the
lock, closing a lock in these circumstance may have different effects,
for example:

- The lock is forcibly released before closing; this should be harmless.
- The closure of the lock fails and is left lying around. When the FILE
object is reused and the lock re-initialised, then the lock object could
be lost; hence the memory leak I observed.
- The lock is closed without being released; this may result in
unexpected/undefined behaviour.

As I mentioned in my original submission, the taking of the FILE lock
should be unnecessary since the global lock has already been taken and
so the state of the FILE should not change while it is being checked to
see if it is in use (twice). Also, after checking the source base, the
fwalk functions are only called with functions which call _flockfile()
and _funlockfile() internally so these "exterior" calls are unnecessary.

The only exception seems to be the call to lflush() by __srefill_r()
when used by the getc() and putc() macro functions (in stdio.h). However
these macro functions do not seem to be generally thread safe anyway,
since no calls to _flockfile() and _funlockfile() are made in the
__sputc_r()/__sgetc_r() macros that they call:

#ifndef __CYGWIN__
#ifndef lint
#define getc(fp)        __sgetc_r(_REENT, fp)
#define putc(x, fp)     __sputc_r(_REENT, x, fp)
#endif /* lint */
#endif /* __CYGWIN__ */

I think the implementations of these macro functions should be updated
to lock the FILE object but this will probably require the _sgetc_r()
and _sputc_r() macros to be implemented as functions.

>> Even though the patches are against a pre- 1.15.0 version of newlib they
>> are still valid against the current wave front.
>>
>> Cheers,
>>
>> Antony.

Cheers,

Antony.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]