GNOME Bugzilla – Bug 704439
Enhancing Thread-Safety of LibXML.
Last modified: 2021-07-05 13:20:42 UTC
Created attachment 249469 [details] [review] LibXML thread-safety patch. Modified parser.c Problem Description: As of now, LibXML (latest version 2.9.1) is not threadsafe, its cleanup functions are very risky to use. It is mentioned in Cleanup function of XML also, that to call cleanup only when the process is exiting, otherwise it may lead to undesired behaviour (crashes). Its very old issue oh XML. People have faced crashes like: 326.337487] REGS: FP: 0x9486993c, PC: 0x419f2744, RA: 0x419f2744,SP: 0x948693d8, Stack End: 0x94869f68 [ 326.357318] #0 0x419f21d0 in vfprintf () from /mtd_exe/lib/libc-2.14.1.so [ 326.365560] #1 0x4106f87c in xmlGenericErrorDefaultFunc () from /mtd_exe/Comp_LIB/libxml2.so.2 [ 326.374630] #2 0x4106ff28 in ?? () [ 326.378498] #3 0x41070ef8 in __xmlRaiseError () from /mtd_exe/Comp_LIB/libxml2.so.2 [ 326.387080] #4 0x41077840 in ?? () [ 326.391028] #5 0x41096164 in xmlParseDocument () from /mtd_exe/Comp_LIB/libxml2.so.2 [ 326.398883] #6 0x4109fb88 in ?? () [ 326.403023] #7 0x4109fd50 in xmlReadMemory () from /mtd_exe/Comp_LIB/libxml2.so.2 Cause: Such issues occur in Multi-threading environment. If some thread has already called XML Cleanup while some other thread is still using XML. In such cases behaviour of XML is undesired. Patch Description: Implemetation of Reference counting in Init and Cleanup of XML. This will increase thread-safety of LibXML. Please Review and apply the attached patch for LibXML Thread-safety. I hope this patch definitely improves LibXML thread-safety. I tested this patch with a sample program which creates multiple (200) threads, each thread calls Init and Cleanup randomly.Some random threads even calls cleanup funtion twice. I am attaching this sample program also.
Created attachment 249470 [details] Sample test program used to test the patch provided.
Doesn't work IMHO, as xmlInitParser() doesn't have to be called by client code, and can be called in directly from the API entry points. Since the whole point of your patch is to assume each 'client' will call it only once and we can count those, I'm afraid this patch is not valid. Simply put: do not call xmlCleanupParser() unless you are in one of the few case where this actually makes sense. Daniel
You are right, xmlInitParser() is not called by client code, but this function is entry point for other xml functions. Hence it is called indirectly but always. My point is, if in multi-threaded environment, multiple threads(Applications) are doing Init and performing Cleanup after their task, they are unknown about other threads(application) that might be doing similar tasks.In this scenario crashes may occur if some thread(application) has called its cleanup while other thread(application) is still using it. I have encountered such crashes with present libxml2. In this patch, If init is done once, it will not be done again.But it will increment the count. Also, if some thread(application) has called cleanup, the cleanup function will not be called unless the xmlRefCount variable is decremented to 0, which ensures all threads(application) have completed their tasks. I was able to eliminate the crashes I encountered by applying this patch.
I understand what you tried to do, no need to re-explain. I am saying this is based on a flawed assumption and does not work. It also reinforce the idea that applications could call the global cleanup function safely in multithreaded code and that's just not the case. If a threaded application calls Cleanup it is a bug in the application unless they do so just before calling exit(). It's that simple. It is what the documentation says. Just do it ! Alternatively I will make that function a no-op to get rid of the issue for good. Daniel
Ok thanks for your guidance Daniel.
It will be good if you make Cleanup function no-op in multi-threading case. Gaurav
That's something we discussed briefly on the list. I was still waiting for some kind of miracle solution to the problem, but it's just too hard, making it a noop seems to be the best way forward, thanks, Daniel
You can make it no-op until some miracle solution comes :)
Created attachment 250752 [details] [review] Making xmlCleanupParser() non operational in multi-threading environment.
Daniel, I also faced similar issues some time back. I made cleanup function empty to avoid such issues. I have attached above a simple patch to make xmlCleanupParser() non operational in multi-threading environment.
Created attachment 260197 [details] [review] Ensures xml init in multi-threading environment
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/libxml2/-/issues/ Thank you for your understanding and your help.