Changeset 1985

Show
Ignore:
Timestamp:
05/22/11 05:52:01 (10 years ago)
Author:
jwoithe
Message:

Fix double-free on exit under the new firewire stack. It seems that with the new kernel firewire stack, raw1394_destroy_handle() can take upwards of 20 milliseconds(!) to return. Therefore the IsoHandler?'s disable() call invoked by the IsoTask? (FW_ISORCV or FW_ISOXMT) may not have completed before the "jackd" thread calls ~IsoHandler?(). ~IsoHandler?() thus infers that the handler is still running and calls disable() itself. The practical upshot is that raw1394_destroy_handle() gets called on the same object twice, and a double-free results.

The fix I've implemented is a touch crude, but it appears to work. A mutex is introduced to track the progress of disable(), and this is checked by ~IsoHandler?() before the state of the handler is tested. Any in-progress disable() call is allowed to complete before ~IsoHandler?() tests the state. This prevents the second call of raw1394_destroy_handle() and therefore the double-free cannot occur.

Perhaps as a result of the delays caused by raw1394_destroy_handle(), it seems the handler list can be altered by other threads while updateShadowMapHelper() (called by the IsoTask? threads) is running. A crude test has been added to this function to prevent out-of-range exceptions in most cases.

None of this is particularly elegant but it should work around the double-free issue for the moment. The correct approach is to work out precisely why these concurrency issues are occuring and fix them. However, given that all this will be obsoleted by the in-kernel streaming work at some point in the future, it's arguable that the solution in this patch is sufficient in practice.

Files:

Legend:

Unmodified
Added
Removed
Modified
Copied
Moved
  • trunk/libffado/src/libieee1394/IsoHandlerManager.cpp

    r1970 r1985  
    142142    m_SyncIsoHandler = NULL; 
    143143    for (i = 0, cnt = 0; i < max; i++) { 
     144 
     145        // FIXME: This is a very crude guard against some other thread 
     146        // deleting handlers while this function is running.  While this 
     147        // didn't tend to happen with the old kernel firewire stack, delays 
     148        // in shutdown experienced in the new stack mean it can happen that 
     149        // a handler disappears during the running of this function.  This 
     150        // test should prevent "out of range" exceptions in most cases.  
     151        // However, it is racy: if the deletion happens between this 
     152        // conditional and the following at() call, an out of range 
     153        // condition can still happen. 
     154        if (i>=m_manager.m_IsoHandlers.size()) 
     155            continue; 
     156 
    144157        IsoHandler *h = m_manager.m_IsoHandlers.at(i); 
    145158        assert(h); 
     
    12381251#endif 
    12391252{ 
     1253    pthread_mutex_init(&m_disable_lock, NULL); 
    12401254} 
    12411255 
     
    12991313// raw1394_destroy_handle() will do any iso system shutdown required. 
    13001314//     raw1394_iso_shutdown(m_handle); 
     1315 
     1316// Typically, by the time this function is called the IsoTask thread would 
     1317// have called disable() on the handler (in the FW_ISORCV/FW_ISOXMT 
     1318// threads).  However, the raw1394_destroy_handle() call therein takes 
     1319// upwards of 20 milliseconds to complete under the new kernel firewire 
     1320// stack, and may not have completed by the time ~IsoHandler() is called by 
     1321// the "jackd" thread.  Thus, wait for the lock before testing the state 
     1322// of the handle so any in-progress disable() is complete. 
     1323    if (pthread_mutex_trylock(&m_disable_lock) == EBUSY) { 
     1324        pthread_mutex_lock(&m_disable_lock); 
     1325        pthread_mutex_unlock(&m_disable_lock); 
     1326    } 
    13011327    if(m_handle) { 
    13021328        if (m_State == eHS_Running) { 
     
    13051331        } 
    13061332    } 
     1333    pthread_mutex_destroy(&m_disable_lock); 
    13071334} 
    13081335 
     
    17941821IsoHandlerManager::IsoHandler::disable() 
    17951822{ 
     1823    signed int i, have_lock = 0; 
     1824 
    17961825    debugOutput( DEBUG_LEVEL_VERBOSE, "(%p, %s) enter...\n",  
    17971826                 this, (m_type==eHT_Receive?"Receive":"Transmit")); 
     1827 
     1828    i = pthread_mutex_trylock(&m_disable_lock); 
     1829    if (i == 0) 
     1830        have_lock = 1; 
     1831    else 
     1832    if (i == EBUSY) { 
     1833        // Some other thread is disabling this handler, a process which can 
     1834        // take considerable time when using the new kernel firewire stack.  
     1835        // Wait until it is finished before returning so the present caller 
     1836        // can act knowing that the disable has occurred and is complete 
     1837        // (which is what normally would be expected). 
     1838        debugOutput( DEBUG_LEVEL_VERBOSE, "waiting for disable lock\n"); 
     1839        pthread_mutex_lock(&m_disable_lock); 
     1840        debugOutput( DEBUG_LEVEL_VERBOSE, "now have disable lock\n"); 
     1841        if (m_State == eHS_Stopped) { 
     1842            debugOutput( DEBUG_LEVEL_VERBOSE, "another disable() has completed\n"); 
     1843            pthread_mutex_unlock(&m_disable_lock); 
     1844            return true; 
     1845        } 
     1846        have_lock = 1; 
     1847    } 
    17981848 
    17991849    // check state 
    18001850    if(m_State != eHS_Running) { 
    18011851        debugError("Incorrect state, expected eHS_Running, got %d\n",(int)m_State); 
     1852        if (have_lock) 
     1853            pthread_mutex_unlock(&m_disable_lock); 
    18021854        return false; 
    18031855    } 
     
    18191871    debugOutput( DEBUG_LEVEL_VERBOSE, "(%p, %s) stop...\n",  
    18201872                 this, (m_type==eHT_Receive?"Receive":"Transmit")); 
     1873 
    18211874    // stop iso traffic 
    18221875    raw1394_iso_stop(m_handle); 
     
    18281881    raw1394_iso_shutdown(m_handle); 
    18291882 
     1883    // When running on the new kernel firewire stack, this call can take of 
     1884    // the order of 20 milliseconds to return, in which time other threads 
     1885    // may wish to test the state of the handler and call this function 
     1886    // themselves.  The m_disable_lock mutex is used to work around this. 
    18301887    raw1394_destroy_handle(m_handle); 
    18311888    m_handle = NULL; 
     
    18331890    m_State = eHS_Stopped; 
    18341891    m_NextState = eHS_Stopped; 
     1892 
     1893    if (have_lock) 
     1894        pthread_mutex_unlock(&m_disable_lock); 
    18351895    return true; 
    18361896} 
  • trunk/libffado/src/libieee1394/IsoHandlerManager.h

    r1943 r1985  
    225225            int m_switch_on_cycle; 
    226226 
     227            pthread_mutex_t m_disable_lock; 
     228 
    227229        public: 
    228230            unsigned int    m_packets;