Exiv2 / exiv2

Image metadata library and tools
http://www.exiv2.org/
Other
932 stars 281 forks source link

add namespace to XMP toolkit - exiv2 and libexempi in same process coredumps #459

Open whatdoineed2do opened 6 years ago

whatdoineed2do commented 6 years ago

Is the possible to wrap the internal xmpsrc XMP toolkit within an Exiv2::internal namespace or similar - since it appears to be only used internally and its interface never exported - to avoid potential translation unit/name clashes with other libraries that also have their own internal XMP toolkit?

The use case/problem scope/justification below:


I have an eog (Eye of Gnome, image viewer) plugin that uses Exiv2 to add XMP ratings to image files as I browse them. This plugin worked for me for many years and most recently on my Fedora26 systems.

With the Fedora 28 release of eog I find that the plugin coredumps/SEGVs when calling Exiv2::TiffImage::readMetaData() and in XMPData::RegisterNamespace() where sNamespaceURIToPrefixMap == NULL

#0  0x00007efd47cd35b8 in XMPMeta::RegisterNamespace(char const*, char const*) () at /lib64/libexiv2.so.26
#1  0x00007efd47cb1537 in WXMPMeta_RegisterNamespace_1 () at /lib64/libexiv2.so.26
#2  0x00007efd47ca0ed8 in TXMPMeta<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::RegisterNamespace(char const*, char const*) () at /lib64/libexiv2.so.26
#3  0x00007efd47c9cf76 in Exiv2::XmpParser::initialize(void (*)(void*, bool), void*) ()
    at /lib64/libexiv2.so.26
#4  0x00007efd47c9f779 in Exiv2::XmpParser::decode(Exiv2::XmpData&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) () at /lib64/libexiv2.so.26
#5  0x00007efd47c87939 in Exiv2::Internal::TiffDecoder::decodeXmp(Exiv2::Internal::TiffEntryBase const*) ()
    at /lib64/libexiv2.so.26
#6  0x00007efd47c7091e in Exiv2::Internal::TiffDirectory::doAccept(Exiv2::Internal::TiffVisitor&) ()
    at /lib64/libexiv2.so.26
#7  0x00007efd47c79653 in Exiv2::Internal::TiffParserWorker::decode(Exiv2::ExifData&, Exiv2::IptcData&, Exiv2::XmpData&, unsigned char const*, unsigned int, unsigned int, void (Exiv2::Internal::TiffDecoder::*(*)(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int, Exiv2::Internal::IfdId))(Exiv2::Internal::TiffEntryBase const*), Exiv2::Internal::TiffHeaderBase*) ()
    at /lib64/libexiv2.so.26
#8  0x00007efd47c7978b in Exiv2::TiffParser::decode(Exiv2::ExifData&, Exiv2::IptcData&, Exiv2::XmpData&, unsigned char const*, unsigned int) () at /lib64/libexiv2.so.26
#9  0x00007efd47c79851 in Exiv2::TiffImage::readMetadata() () at /lib64/libexiv2.so.26
...

After some investigation I find that eog uses libexempi (which includes its own copy of the XMP toolkit) which when used in conjunction with exiv2’s XMPtool in the same process space causes problems - it appears both are trying to initialise their own versions of the their XMPtool kit but because of variable name clashes exiv2 is left unitialised leading to sNamespaceURIToPrefixMap == NULL and the SEGV.

I have a simple test case that demonstrates the problem:

#include <iostream>
#include <stdexcept>
#include <exiv2/exiv2.hpp>
#include <exempi-2.0/exempi/xmp.h>

int main(int argc, char* argv[])
{
    if (argc != 2) {
    std::cerr << "needs ONE image file" << std::endl;
    return -1;
    }

    try
    {
    xmp_init();

    Exiv2::Image::AutoPtr  img = Exiv2::ImageFactory::open(argv[1]);
    img->readMetadata(); 
    }
    catch (const std::exception& ex)
    {
    std::cerr << ex.what() << std::endl;
    }
    return 0;
}

On F28, libexempi -> libexempi.so.3 and we can see the problem if libexempi is initialised/loaded first by the rt linker.

LD_PRELOAD=/usr/lib64/libexempi.so.3 ./a.out /tmp/foo.NEF

I don't think it matters what the file is as long as it has XMP data but my Nikon NEFs trigger the problem. Removing the xmp_init() call we we don't have the problem.

Given that this appears to be a conflict with names for the 2 XMP toolkits, are you open to wrapping Exiv xmpsrc within an Exiv2::internal namespace? I think this would solve the problem and ensure there are no such conflicts with other instances of XMP toolkits in frameworks/programs?

rdieter commented 6 years ago

It could be argued libexempi should do the same (namespace their bundled xmp toolkit library too).

clanmills commented 6 years ago

I did look at exempi last year. I don't recall why we decided not to adopt that as a build option for Exiv2.

We decided to support Adobe XMPsdk has an external library and that will be offered in Exiv2 v0.27. I'm hoping to publish Exiv2 v0.27 RC1 next week when I return from vacation. Several users asked us to "externalise" Adobe XMPsdk. For v0.27, you can choose: 1) No XMP support 2) To use the existing code that's statically linked into Exiv2 (and causes entry point collisions with XMPsdk) 3) To build Adobe's XMPsdk 2016 and link it into Exiv2.

I don't know if that answers your question, however you can build this with the current 'master'. How to build this is documented in README-CONAN.md.

One of our users discussed this with me about 3 years ago and I suggested that he wrap the XMPsdk statics in the Exiv2 namespace to prevent collisions with Adobe's library. He succeeded in getting that to work, however he said it was "messy" and he didn't want to contribute his fix into the public code base of Exiv2. I didn't discuss his "fix" in any more detail.

If you believe that exempi can be used as an alternative to Adobe XMPsdk, you are welcome to join Team Exiv2 and develop/contribute to v0.28.

whatdoineed2do commented 6 years ago

Thanks for the input Robin

As exiv2 is allowing external XMPsdk builds I think options are limited. Initial thoughts would be to wrap any statics in an Exiv2 namespace as you suggested to the other user. I think it I found 6 in total so a quick sed will do it. However, I'm not sure how this would be done for the conan retrieved code unless we have a hook to perform the same statics renaming/cleanup.

Additionally (and its been a while since I was hacking at this low level) I can't remember what happens when we have the same symbol names/functions in 2 different shared libraries and something calls for that function - in my situation, libexempi has the XMPData::Initialize() and XMPData::RegisterNamespace() methods as does libexiv2 .. so what happens when we call RegisterNamesapce() which now is refering to statics that have been renamed. Is it possible that one call goes to libexempi and another goes to libexiv2.

My core dump looks like it happened because XMPsdk's XMPData::initialize() uses the static sNamespaceURIToPrefixMap variable which was initialised in the libexempi "scope" but not in libexiv2. When the code called into the libexiv2's XMPData::RegisterNamespace(), this var was uninitialised and hence null and hence SEGV. If the RTLD used the function in the libexempi address space, it would have been ok and that static would already be initialised.

All of this points to we'd still put all of the XMPsdk into an Exiv2 namesapce to be safe: this would have to happen for the internal XMPsdk or the external XMPsdk pulled from conan.

Is that a simple thing? Is there a façade between exiv2 and the XMPsdk stuff? Moving to using libexempi would require such a fascade as it seems to only provide these development headers:

/usr/include/exempi-2.0/exempi
/usr/include/exempi-2.0/exempi/xmp++.hpp
/usr/include/exempi-2.0/exempi/xmp.h
/usr/include/exempi-2.0/exempi/xmpconsts.h
/usr/include/exempi-2.0/exempi/xmperrors.h
clanmills commented 6 years ago

I don't think I'm willing to do anything about this. For certain, I won't look at this before we release Exiv2 v0.27. I'm hoping to publish RC1 in October when I get home from vacation and RC2 in November. GM possibly by Christmas - it depends on user feedback and the run-of-the-mill bug reports that arrive during the RC period.

I would be really wonderful to have an engineer on Team Exiv2 to specialise and "own" XMP support. Being able to build and link Adobe XMPsdk 2016 was worked on by Ben, myself and @piponazo I'm delighted that @piponazo finished the job and integrated everything with CMake/conan. A really great job.

However that job isn't totally finished, as it requires more testing. There is also an outstanding issue about supporting 'extended XMP' in which the XMP/xml exceeds 64k bytes #396 I would like to see that feature being delivered in Exiv2 v0.27 GM.

I feel a request for Exiv2 to work in conjunction with exempi can't be undertaken at this time. Without an engineer joining us and accepting the mission of being "Team Exiv2 XMP Expert", I think it's unlikely that this matter will be pursued in 2019.

cgilles commented 6 years ago

Some feedback about this problem with digiKam.

2 years ago, we have seen exactly the same problem about the XMP SDK namespace. In fact, the problem is not really the namespace to add around this Adobe code. It's a symbols export problem.

To be more precise :

1/ digiKam has an old XMP SDK inside, to compile the DNG SDK from adobe. The DNG sdk is used to convert RAW files to DNG. Of course, the Adbode puzzle force to use an old XMP SDK with the DNG SDK, as the compatibility is not preserver between the version of both. Sound like Adobe has 2 teams not working together. But it's another story...

2/ Exiv2 use another XMP SDK inside, different than one from digiKam core.

When we port whole digiKam from Qt4 to Qt5, with the migration from the DNG converter tool as an external plugin to a code inside digiKam core (not protected by 2 compilation process), many linking failure appears everywhere.

After a long investigations, we found a solution to solve this issue without to touche the XMP DSK code including a namespace wrapper. In fact, we use some new feature from cmake to reduce the symbols visibility as well for internal static library including the SDK. This prevent to mix Exiv2 symbols and DK core symbols.

https://cgit.kde.org/digikam.git/tree/core/libs/dngwriter/CMakeLists.txt#n160

Look the PRIVATE keyword used to link the static library. This one is used later to link the target application, with Exiv2 : https://cgit.kde.org/digikam.git/tree/core/app/CMakeLists.txt#n146 This one is the share libdigikam used by target application. Look the Exiv2 and libdng rules mixed here.

Voilà... Gilles Caulier

clanmills commented 6 years ago

Merci beaucoup mon ami, M Caulier.

I'll have a look at this next week when I get home from vacation. And the stuff you wrote about on Sunday. http://dev.exiv2.org/issues/1347

I'm interested in wrapping the statics in the adobe code built into Exiv2 (in xmpsdk/src). I'm not enthusiastic about changing anything in Adobe XMPsdk/2016.

By the way, it was me (not Andreas) who gave you the nickname "Human Dynamo" in the release notes for Exiv2 v0.24. You deserve that name because your effort appears inexhaustible.

We're in Northumberland this evening. Went for a run that finished at Bamburgh Castle a few minutes before sun-set. Life is great. Photo is untouched (only rescaled).

img_1501

clanmills commented 5 years ago

I've assigned this to milestone v0.28. We're now at Exiv2 v0.27 RC3 and the demands of the release have left no time to investigate this. After Exiv2 v0.27 is released on 28 December 2018, I will focus on "dot" releases (security updates) to Exiv2 v0.27.

It's up to the community to investigate and consider action about this matter.

clanmills commented 3 years ago

Exiv2 v1.00 is schedule for 2021-12-15. One of the projects being considered is to remove XMPsdk from the code base and handle as an external dependency as we do for other libraries such as expat (for XMPsdk) or zlib (for PNG). This might solve this issue. #1515.

When XMPsdk is an external library, any collision with exempi in a process will not be caused by Exiv2.