Bug #1330
Crash in Exiv2::TiffImage::readMetadata
100%
Description
Hello,
there is a darktable user reporting a crash on Windows which looks like some problem inside exiv2 to me. The backtrace is here: https://pixls-discuss.s3.amazonaws.com/original/2X/e/ec1ac0f71e2df63696d9446ca3e640f965d25167.txt
The relevant part to me is that we use Exiv2::TiffImage::readMetadata which seems to use Exiv2::TiffImage::printStructure internally. However, that is not thread safe according to your documentation.
Files
History
Updated by Robin Mills almost 4 years ago
- Category set to tiff parser
- Status changed from New to Closed
- Assignee set to Robin Mills
- Target version set to 0.27
- % Done changed from 0 to 100
- Estimated time set to 1.00 h
Tobias
Regrettably, I don't have good news for you here. The good news is that we know all about this. Unfortunately, there isn't a simple "one commit" fix. There is a multi-threading issue with the code. However, there are numerous other issues with the printStructure() code and have revealed several security issues with Exiv2. We've been very busy since September dealing with them.
Realistically, the fix is to move to v0.27 when it is released. I can't honestly say when that will be ready. I'm suffering from motivational issues. Mostly, I'm getting old (67 next month) and would like to retire. 2017 has been an exhausting year with Exiv2. For the first time, I actually published a release and found that a very tough experience. We moved the code to Git and, at the moment, I find it effectively impossible to submit code to GitHub.com Then the Linux security people arrived and have been dumping security issues on us. I've been travelling in the States and Vietnam and several security bugs every week was very distressing. They are using "fuzzing tools" which generate illegal files which exploit weaknesses in the code (including the one you have reported here).
The good news is that I have recruited three great new engineers who have put in lots of work on the security and other issues.
I'm hoping that after Christmas, I'll break out of my current mood, get the 0.27 release ready, and release it spring 2017. While travelling, I thought "Oh when I get home in December, I'll deal with this.". I'm now home (with no plan to travel until August 2018). I am doing some work, however I remain very demotivated.
As you know, open source puts quite extra-ordinary demands onto a small number of people. I'm very appreciative of my new team mates. I'm also pleased with the move to git. After 5 years of dealing almost alone with Exiv2, I now have more engineers. Moving the code to GitHub was the magnet to attract fresh contributors. However, I remain "the man" to do the release. I wish I were not.
I'm going to close this issue because I cannot give you a simple response. I assure that this matter is fixed on 'master', however 'master' is not ready to be released at this time.
Updated by Tobias E. almost 4 years ago
I understand you. Both the issue at hand and also your general problems with being responsible for an open source project. My advantage is that I am working on the top of the food chain an end user application. Doing infrastructure like you is extra hard as everyone complains. Like me. :-)
So thank you for your open and honest words. I will look forward to any new release and try to come up with ways to mitigate the crashes in the mean time. Worst case would be a bunch of locks in the code.
Keep up the good work and have a nice Christmas time with the family!
Updated by Robin Mills almost 4 years ago
Tobias
I don't think many folks complain. Almost the opposite - lots of praise and encouragement. Things are as they are. We'll get there. Tomorrow I'll construct a patch against the released v0.26. If it passes the test suite, I'll send it to you. There's a possibility we can fix this with one good patch. However, if the patch doesn't work out, I feel you have to wait for v0.27. Let's see if we can fix up something in the next few days.
Robin
Updated by Robin Mills almost 4 years ago
- File forTobias0-26-MultithreadingPatch.patch forTobias0-26-MultithreadingPatch.patch added
- Status changed from Closed to Assigned
- % Done changed from 100 to 50
- Estimated time changed from 1.00 h to 4.00 h
I attach a patch for you to try.
Incidentally, there is a multi-threading samples "samples/mt-test.cpp". It's not built by default because it uses C++11 (which is slated for v0.27), however the code is in v0.26. It can read 100 images on my laptop on both the "released v0.26" and this patched code.
The "unthread safe" code in tiffimage.cpp is subtle. Like most multi-threading issues, it's hard to consistently reproduce!
Updated by Tobias E. almost 4 years ago
Wow, that was fast! I gave the patch a try using the sample code you mentioned, unfortunately it doesn't crash for me. The fun of race conditions.
For now I added some locks around calls to readMetadata in darktable. That way it should work for people using stock exiv2, too. I will keep testing your patch myself.
Thanks again.
Updated by Robin Mills almost 4 years ago
- % Done changed from 50 to 100
- Estimated time changed from 4.00 h to 2.00 h
Ah yes of course. You can lock around readMetadata() in the application code. Yes, that sounds like a good work-around for now.
Best Wishes to you and your for Christmas and New Year.