mono / taglib-sharp

Library for reading and writing metadata in media files
GNU Lesser General Public License v2.1
1.29k stars 312 forks source link

When running multithreaded, sometimes CRC calculations for PNG fail #159

Open grisharav opened 5 years ago

grisharav commented 5 years ago

When I run in parallel lots of File.Create(...) calls (on different paths) I sometimes get

TagLib.CorruptFileException: CRC check failed for IHDR Chunk (expected: 0xFFFFFFFE, read: 0x5446E2B7 at TagLib.File.Create(IFileAbstraction abstraction, String mimetype, ReadStyle propertiesStyle)

on the first file

I believe this is due to unsafe code in File.cs:

    static ByteVector ComputeCRC (params ByteVector[] datas)
    {
        uint crc = 0xFFFFFFFF;

        if (crc_table == null)
            BuildCRCTable ();

        foreach (var data in datas) {
            foreach (byte b in data) {
                crc = crc_table[(crc ^ b) & 0xFF] ^ (crc >> 8);
            }
        }

            // Invert
        return ByteVector.FromUInt (crc ^ 0xFFFFFFFF);
    }

in the if (crc_table==null) check, multiple threads can start building the crc table and corrupt the results.

Starwer commented 5 years ago

I think you're right. A simple fix for this would be to assign the crc_table onlly at the end of the function BuildCRCTable instead of initializing it at the begining, i.e.

        static void BuildCRCTable ()
        {
            uint polynom = 0xEDB88320;

            //crc_table = new uint[256];
            var table = new uint[256];

            for (int i = 0; i < 256; i++) {

                uint c = (uint)i;
                for (int k = 0; k < 8; k++) {
                    if ((c & 0x00000001) != 0x00)
                        c = polynom ^ (c >> 1);
                    else
                        c = c >> 1;
                }
                //crc_table[i] = c;
                table[i] = c;
            }
            crc_table = table;

        }
grisharav commented 5 years ago

Will probably work, I would simply make crc_table a Lazy<uint[]>

Starwer commented 5 years ago

Good idea! This be even cleaner.