CYG_ASSERTCLASS bug in mboxt2.inl?

CYG_ASSERTCLASS bug in mboxt2.inl?

[Berend Ozceri]

The mbox implementation template mboxt2.inl from CVS contains the
following code:

        #ifdef CYGMFN_KERNEL_SYNCH_MBOXT_PUT_CAN_WAIT
        template <class T, cyg_count32 QUEUE_SIZE>
        CYG_MBOXT_INLINE cyg_bool
        Cyg_Mboxt2<T,QUEUE_SIZE>::put( const T item )
        {
        ...
            if ( !get_threadq.empty() ) {
                wakeup_winner( item );
                Cyg_Scheduler::unlock();        // unlock, maybe switch threads
                CYG_ASSERTCLASS( this, "Bad this pointer");    
                CYG_REPORT_RETVAL( true );
                return true;
            }
        ...

Isn't the placement of the CYG_ASSERTCLASS in the !get_threadq.empty()
case erroneous? Consider what happens if there indeed was a thread with
a higher-priority waiting on a cyg_mbox_get() (or variant) when another
thread with a lower-priority calls the put() method listed above. As the
comment indicates, the Cyg_Scheduler::unlock() call will cause a thread
switch to occur at that point because the higher priority thread can now
complete its get() operation.

Now, further consider the case where the action that the woken up thread
takes immediately (or soon after) waking up is the destruction of this
mailbox, including freeing the memory used by it. Some time after that,
the original thread that called put() will get a chance to run and enter
the CYG_ASSERTCLASS() check, but at this point the object data is
invalid and the assertion fails.

It seems that since the only code path that remains for the original
caller to the put() function to complete is returning after waking up,
this sort of a synchronous-deletion of the mailbox by the getter while
the putter may still be in a method call should be safe, but the
placement of the CYG_ASSERTCLASS() makes it not so. As a matter of fact,
the lwIP code's use of mailboxes in api_lib.c/api_msg.c is susceptible
to this behavior and the code works correctly when assertions are
disabled, but fails with false-positives when they are enabled.

So, was this an intentional placement of CYG_ASSERTCLASS(), effectively
requiring the use of a secondary mutex to control the creation and
destruction of the mailbox object itself when the producer and consumer
threads are of different priorities or should it be moved two lines up
to be before the Cyg_Scheduler::unlock() call?

Berend