processExifTag in Metadata is not thread-safe

Help
2012-10-03
2012-11-29
  • Corey Taylor

    Corey Taylor - 2012-10-03

    After upgrading to FreeImage 3.15.3, I ran a test to check threaded loading of
    jpeg images. I immediately ran into a crash caused by processExifTag and
    TagLib.

    The issue is occurs when two threads try to load the first set of jpeg files
    for the application. If they end up calling processExifTag, that will invoke
    the TagLib constructor through:

    TagLib& s = TagLib::instance();
    

    Which contains:

    TagLib& 
    TagLib::instance() {
        static TagLib s;
        return s;
    }
    

    The problem is nothing gates the threads from waiting for the object to finish
    initalization before using it. You can end up with two callstacks that look
    like:

        scratch-coreyd.exe!std::map<unsigned short,tagTagInfo *,std::less<unsigned short>,std::allocator<std::pair<unsigned short const ,tagTagInfo *> > >::operator[](const unsigned short & _Keyval)  Line 215 + 0x38 bytes   C++
        scratch-coreyd.exe!TagLib::addMetadataModel(TagLib::MDMODEL md_model, tagTagInfo * tag_table)  Line 1490 + 0x1b bytes   C++
        scratch-coreyd.exe!TagLib::TagLib()  Line 1449  C++
    >   scratch-coreyd.exe!TagLib::instance()  Line 1513 + 0x2a bytes   C++
        scratch-coreyd.exe!processExifTag(FIBITMAP * dib, FITAG * tag, char * pval, long msb_order, TagLib::MDMODEL md_model)  Line 495 + 0x5 bytes C++
    

    and

        scratch-coreyd.exe!std::_Tree<std::_Tmap_traits<int,std::map<unsigned short,tagTagInfo *,std::less<unsigned short>,std::allocator<std::pair<unsigned short const ,tagTagInfo *> > > *,std::less<int>,std::allocator<std::pair<int const ,std::map<unsigned short,tagTagInfo *,std::less<unsigned short>,std::allocator<std::pair<unsigned short const ,tagTagInfo *> > > *> >,0> >::find(const int & _Keyval)  Line 1424 + 0x10 bytes   C++
        scratch-coreyd.exe!TagLib::getTagInfo(TagLib::MDMODEL md_model, unsigned short tagID)  Line 1520 + 0x36 bytes   C++
        scratch-coreyd.exe!TagLib::getTagFieldName(TagLib::MDMODEL md_model, unsigned short tagID, char * defaultKey)  Line 1533 + 0x11 bytes   C++
        scratch-coreyd.exe!processExifTag(FIBITMAP * dib, FITAG * tag, char * pval, long msb_order, TagLib::MDMODEL md_model)  Line 500 + 0x15 bytes    C++
    

    I am obviously skipping a few lines since they are huge to show you the issue.

     
  • Corey Taylor

    Corey Taylor - 2012-10-04

    I have patched this for our copy as follows:

    static volatile bool isTagLibInitialized = false;
    
    /**
    This is where the tag info tables are initialized
    */
    TagLib::TagLib() {
    
    ...
    
        isTagLibInitialized = true;
    
    TagLib& 
    TagLib::instance() {
        static TagLib s;
    
        // wait until constructor is finished in-case it's happening on another thread
        while (! isTagLibInitialized) {
            // Query time as a busy wait
            time_t t;
            time(&t);
        }
    
        return s;
    }
    

    I made it as generic as possible.

     
  • Corey Taylor

    Corey Taylor - 2012-10-04

    it needs a better solution, but this patches the issue for me with the busy
    wait including the call to time() which lets the constructor finish.

    The timing was rare enough, but so far I've not see it execute the
    instructions to check if the static is set or not at the same time.

     
  • Corey Taylor

    Corey Taylor - 2012-10-04

    I should clarify that the crash timing was rare for us, but once I hit it
    locally, I could reproduce it pretty easily in a debugger with breakpoints or
    thread switching. However, our case involved spawning threads to start the
    image loading, so I could see the timing being different when using more
    existing threads.

    However, excluding the static initialization, the volatile should work in this
    case because there isn't any re-ordering that would affect the logic. I just
    need it to spin X number of times while checking.

     
  • Hervé Drolon

    Hervé Drolon - 2012-10-07

    Hi,

    Maybe we could use FreeImage_Initialise to setup the TagLib ?
    Since this funtion is called a single time, before any other thread is
    launched, that could solve the problem.

    Hervé

     
  • Corey Taylor

    Corey Taylor - 2012-10-07

    I think putting it in FreeImage_Initialise would work. It's up to the
    application to ensure that's called correctly.

     
  • Corey Taylor

    Corey Taylor - 2012-11-29

    FYI,

    We have not run into this issue after applying the final patch proposed by Herve.

    corey

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks