This is the mail archive of the ecos-discuss@sources.redhat.com mailing list for the eCos 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]

Thread safe in libc file stream



It seems to me that the function cyg_libc_stdio_flush_all_but(
Cyg_StdioStream *not_this_stream ) is not thread-safe.

By a quick comparison between this function and fopen()/fclose() functions,
it appears that fopen/fclose correctly lock and unlock the global(static)
file stream table by calling     Cyg_libc_stdio_files::lock()/unlock()
around the call of Cyg_libc_stdio_files::get_file_stream(i). But this is not
done in the cyg_libc_stdio_flush_all_but().

So, any place sscanf() is called(whose internal implementation will call
cyg_libc_stdio_flush_all_but()), it could cause uncontrolled access to the
global file stream table, and might turn out working on an invalid file
stream pointer.


One option is to protect it by lock/unlock:

        for (i=0; (i<FOPEN_MAX) && !err; i++) {
            if (files_flushed[i] == false) {
			Cyg_libc_stdio_files::lock()
	            stream = Cyg_libc_stdio_files::get_file_stream(i);
      	      if ((stream == NULL) || (stream == not_this_stream)) {
				... ...
	            } // if
      	      else {
				... ...
	            } // else
			Cyg_libc_stdio_files::unlock()
		}
	  }

But this option seems to cause deadlock!?#?!


The second option bases on the answer to this question: why need this
function cyg_libc_stdio_flush_all_but()?

a quick search through the code found out it is called in the following
function:

Cyg_ErrNo
Cyg_StdioStream::refill_read_buffer( void )
{
    Cyg_ErrNo read_err;
    cyg_uint8 *buffer;
    cyg_uint32 len;

    CYG_ASSERTCLASS( this, "Stream object is not a valid stream!" );

    if (!lock_me())
        return EBADF;  // assume file is now invalid

    // first just check that we _can_ read this device!
    if (!flags.opened_for_read) {
        unlock_me();
        return EINVAL;
    }

#ifdef CYGSEM_LIBC_STDIO_WANT_BUFFERED_IO
    // If there is pending output to write, then this will check and
    // write it
    if (flags.buffering) {
        read_err = flush_output_unlocked();

        // we're now reading
        flags.last_buffer_op_was_read = true;

        // flush ALL streams
        if (read_err == ENOERR)
------>     read_err = cyg_libc_stdio_flush_all_but(this);

        if (read_err != ENOERR) {
            unlock_me();
            return read_err;
        } // if

        len = io_buf.get_buffer_addr_to_write( (cyg_uint8**)&buffer );
        if (!len) { // no buffer space available
            unlock_me();
            return ENOERR;  // isn't an error, just needs user to read out
data
        } // if
    }
    else
#endif

    if (!flags.readbuf_char_in_use) {
        len = 1;
        buffer = &readbuf_char;
    }
    else {
        // no buffer space available
        unlock_me();
        return ENOERR;  // isn't an error, just needs user to read out data
    } // else

    read_err = cyg_stdio_read(my_device, buffer, &len);


#ifdef CYGSEM_LIBC_STDIO_WANT_BUFFERED_IO
    if (flags.buffering)
        io_buf.set_bytes_written( len );
    else
#endif
        flags.readbuf_char_in_use = len ? 1 : 0;

    unlock_me();

    if (read_err == ENOERR) {
        if (len == 0) {
            read_err = EAGAIN;
            flags.at_eof = true;
        }
        else
            flags.at_eof = false;
    } // if

    return read_err;
} // refill_read_buffer()

It looks like the "flush ALL streams" potion of code is something extra to
the purpose of this function. My understanding is that this is trying to
provide some background flush for the file streams. But it turns out anytime
one specific stream is trying to read in something, it has to take the
burden of trying to flush All other streams. And this attempt is a full
range loop through the global file stream table. By default, the size of
that table is 8, but could be some number much larger. And it also could be
a double loop. What makes the matter worse is that sscanf() also has to take
the same burden, and because
cyg_libc_stdio_flush_all_but(this) is not thread safe, ......

All this raises the question of if this is the appropriate place for this
extra stuff. It seems to cause some unnecessary dependency.

A quick review of the code finds out that file stream will flush itself
whenever it refills the read buffer or writes something to it. So as long as
the caller make sure he/she explicitly flushes or closes the stream or
attach a '\n' after the last write, it should be fine without help from the
third party. :)


If this is not acceptable to some people, then the flush_all has to be
thread-safe. One step further is that the loop should be optimized(only
flush a limit number of streams in each attempt). I also do not quite
understand the purpose of the double attempt(loop) in the flush_all. It
seems to try to flush as many as possible in one function call, but
definitely with a cost. By the way, since it is called in the
refill_read_buffer(), I doubt if the system sits idle, no stream is
operating for a while, there is still chance the last write to some streams
would not be flushed. Maybe this should be called in the idle thread.

Please let me know if I miss anything.






Following if the copy of fopen() and cyg_libc_stdio_flush_all_but()

ecos/packages/language/c/libc/stdio/current/src/common/fopen.cxx(rev 1.5)

static FILE *fopen_inner( cyg_stdio_handle_t dev,
                          Cyg_StdioStream::OpenMode open_mode,
                          cyg_bool binary,
                          cyg_bool append)
{
    Cyg_StdioStream *curr_stream;
    int i;
    Cyg_ErrNo err;
    int bufmode = _IOFBF;
    cyg_ucount32 bufsize = BUFSIZ;

    Cyg_libc_stdio_files::lock();

    // find an empty slot
    for (i=0; i < FOPEN_MAX; i++) {
        curr_stream = Cyg_libc_stdio_files::get_file_stream(i);
        if (curr_stream == NULL)
            break;
    } // for

    if (i == FOPEN_MAX) { // didn't find an empty slot
        errno = EMFILE;
        cyg_stdio_close( dev );
        Cyg_libc_stdio_files::unlock();
        return NULL;
    } // if

    // Decide the buffering mode. The default is fully buffered, but if
    // this is an interactive stream then set it to non buffered.
    if( (dev != CYG_STDIO_HANDLE_NULL) &&
        cyg_stdio_interactive( dev ) )
        bufmode = _IONBF, bufsize = 0;

    // Allocate it some memory and construct it.
    curr_stream = (Cyg_StdioStream *)malloc(sizeof(*curr_stream));
    if (curr_stream == NULL) {
        cyg_stdio_close( dev );
        Cyg_libc_stdio_files::unlock();
        errno = ENOMEM;
        return NULL;
    } // if

    curr_stream = new ((void *)curr_stream) Cyg_StdioStream( dev, open_mode,
                                                             append, binary,
                                                             bufmode,
bufsize );
    // it puts any error in its own error flag
    if (( err=curr_stream->get_error() )) {

        Cyg_libc_stdio_files::unlock();

        free( curr_stream );

        cyg_stdio_close( dev );

        errno = err;

        return NULL;

    } // if

    Cyg_libc_stdio_files::set_file_stream(i, curr_stream);

    Cyg_libc_stdio_files::unlock();

    return (FILE *)(curr_stream);

} // fopen_inner()




ecos/packages/language/c/libc/stdio/current/src/common/fflush.cxx(rev 1.6)

// flush all but one stream
externC Cyg_ErrNo
cyg_libc_stdio_flush_all_but( Cyg_StdioStream *not_this_stream )
{
    cyg_bool files_flushed[FOPEN_MAX] = { false }; // sets all to 0
    cyg_bool loop_again, looped = false;
    cyg_ucount32 i;
    Cyg_ErrNo err=ENOERR;
    Cyg_StdioStream *stream;

    do {
        loop_again = false;

        for (i=0; (i<FOPEN_MAX) && !err; i++) {
            if (files_flushed[i] == false) {

                stream = Cyg_libc_stdio_files::get_file_stream(i);

                if ((stream == NULL) || (stream == not_this_stream)) {
                    // if it isn't a valid stream, set its entry in the
                    // list of files flushed since we don't want to
                    // flush it
                    // Ditto if its the one we're meant to miss

                    files_flushed[i] = true;
                } // if
                else {
                    // valid stream
#ifdef CYGSEM_LIBC_STDIO_WANT_BUFFERED_IO
                    // only buffers which we've written to need flushing
                    if ( !stream->flags.last_buffer_op_was_read)
#endif
                    {
                        // we try to flush the first time through so that
                        // everything is flushed that can be flushed.
                        // The second time through we should just wait
                        // in case some other lowerpri thread has locked the
                        // stream, otherwise we will spin needlessly and
                        // never let the lower pri thread run!
                        if ( (looped && stream->lock_me()) ||
                             stream->trylock_me() ) {
                            err = stream->flush_output_unlocked();
                            stream->unlock_me();
                            files_flushed[i] = true;
                        } // if
                        else {
                            loop_again = true;
                            looped = true;
                        }
                    }
                } // else
            } // if
        } // for
    } // do
    while(loop_again && !err);

    return err;
} // cyg_libc_stdio_flush_all_but()






Yuxin Jiang
Software Engineer
Matrics Inc. (www.matrics.com)
8850 Stanford Boulevard, Suite 3000
Columbia, MD 21045
Tel: 410-872-0300x115
Fax: 410-872-0700
yjiang@matrics.com


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss


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