libimobiledevice / libplist

A library to handle Apple Property List format in binary or XML
https://libimobiledevice.org
GNU Lesser General Public License v2.1
546 stars 305 forks source link

Crash on multithread environment, xmlCleanup causes crash #68

Closed skortela closed 8 years ago

skortela commented 8 years ago

plist will crash when using from different threads.

xplist.c: methods: plist_to_xml(...) / plist_from_xml(..) are using xml cleanup: xmlCleanupCharEncodingHandlers(); // Cleanup called from thread 1 xmlDictCleanup(); xmlResetLastError(); xmlCleanupGlobals(); xmlCleanupThreads(); xmlCleanupMemory();

crash will happen when another thread will call xmlParseMemory after another has used xmlCleanup functions. static xmlDocPtr new_xml_plist(void) { char *plist = strdup(plist_base); xmlDocPtr plist_xml = xmlParseMemory(plist, strlen(plist)); // CRAHS on thread 2

free(plist);

return plist_xml;

}

When removing cleanup functions, crash will not occur. Can be reproduced easily: thread 1: idevice_get_device_list(...);

thread 2: // wait thread 1 to finish and call: idevice_get_device_list(...); // crash

FunkyM commented 8 years ago

Yes, confirmed. It looks like the only real solution is to introduce a libplist_init|deinit() due to libxml2's API design as without the cleanup memory will leak.

nikias commented 8 years ago

See also #57

nikias commented 8 years ago

Following up on my suggestion with a protecting mutex I didn't take into account that it isn't really solving the problem. @FunkyM you are right with init/deinit. I will take care of that.

greg-dennis commented 8 years ago

Aha, this is causing https://github.com/libimobiledevice/ideviceinstaller/issues/41.

Simply dropping this section would probably be alright. In a multi-threaded environment, libxml recommends either calling xmlCleanupParser once before program exit or simply not calling it at all. See: "In case of doubt abstain from calling this function or do it just before calling exit() to avoid leak reports from valgrind". Even though you may get a leak report from valgrind, I don't see it as a genuine leak. Yes, some global state may hang around for longer than necessary, but I don't think there's some continued accumulation of that memory over time.

If you do add something, I think you only need a deinit, as the lazy init is taken care of for you by libxml.

nikias commented 8 years ago

I accepted the pull request #73 for now to work around this problem.