After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 584605 - Crash in multi-threaded application on Win32, while parsing buffer
Crash in multi-threaded application on Win32, while parsing buffer
Status: RESOLVED FIXED
Product: libxml2
Classification: Platform
Component: general
2.7.3
Other All
: Normal critical
: ---
Assigned To: Daniel Veillard
libxml QA maintainers
Depends on:
Blocks:
 
 
Reported: 2009-06-02 12:17 UTC by Igor
Modified: 2010-12-03 18:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Move initialization of cleanup_helpers_cs to xmlOnceInit (1.04 KB, patch)
2010-12-03 18:28 UTC, Kevin Puetz
none Details | Review

Description Igor 2009-06-02 12:17:38 UTC
Steps to reproduce:
simply call xmlInitParser() from not main thread.
I use 2.7.3 on Win32.


Stack trace:
xmlGetGlobalState() line 654
__xmlGenericError() line 858 + 5 bytes
xmlInitParser() line 13796 + 5 bytes
rtSaxCCreateXmlReader(OSCTXT * 0x0012f724, void * 0x0012f5f8, int (void *,

...

main() line 183 + 9 bytes
mainCRTStartup() line 206 + 25 bytes
KERNEL32! 7c816fe7()


Other information:
The EnterCriticalSection(&cleanup_helpers_cs); in the xmlGetGlobalState() crashes because the cleanup_helpers_cs is not initialized.
xmlInitParser->xmlGenericError->xmlGetGlobalState->EnterCriticalSection
is called before
xmlInitParser->xmlInitThreads->InitializeCriticalSection.

I would propose to fix the xmlInitParser by putting
        xmlInitGlobals();
        xmlInitThreads();
before 
	if ((xmlGenericError == xmlGenericErrorDefaultFunc) ||
	    (xmlGenericError == NULL))
	    initGenericErrorDefaultFunc(NULL);

But such fix requires to remove call to xmlGenericError from within xmlInitThreads().

If you approve such fix I could prepare patch using diff.
Comment 1 Daniel Veillard 2009-06-04 09:12:23 UTC
The problem is that the parser should really be initialized from
the main thread, as explained in http://xmlsoft.org/threads.html

but the changes suggested looks fine to me, so I made the changes
and pushed to GIT head, please double check just in case but I guess
this should clear things.

I'm still unsure of what would happen if you call xmlInitParser from
outside the main thread and try to access libxml2 from the main thread
thereafter, I can't garantee the initialization is correct unfortunately.

Daniel
Comment 2 Igor 2009-06-04 13:38:06 UTC
Thank you for explanation!
To my sorrow the xmlInitParser and xmlCleanupParser are not called from my code,
but from within 3-rd party library I use. In addition this library calls xmlInitParser before each docuemnt parsing and calls xmlCleanupParser immediately after the document was parsed. Therefore I can't ensure that they will be called from one thread.
After I looked in the xmlInitParser again I found the workaround.
What can I do is to call the xmlInitParser from my application on start-up (before the 3-rd party library isinvokated) and to call xmlCleanupParser before my application exits. This should ensure that DeleteCriticalSection will be called only when the critical section is not needed anymore.
I tested this workaround, while useing the libxml2 without patch. It worked.

Therefore, from my point of view the patch is not needed and can be discarded : )

Thanks again!


Comment 3 Igor 2009-06-15 06:20:47 UTC
Hi Daniel,
Thanks for getting patch in!

I have a couple of questions, that were ignored by the xml@gnome.org community. Sorry for using the bugzilla channel, but may be you have answers, or you can help to get the answers ... I'll really appreciate any help on this.
The questions are:
1. Where can I find schedule for next release?
2. I saw VxWorks folders in the on-line project tree, referenced from within libxml2-2.6.32. But the latest release libxml2-2.7.3 doesn’t contain it. Why is it?
3. In case it was decided by maintainers not to take the VxWorks folder, Could I propose my version of Makefile.vxworks?
4. While compiling for VxWorks (using my build framework), I was needed to make a minor fixes (rename the ERROR token in pattern.c and xmlregexp.c files). I would like to see this change in the next version. Should I submit patch using Git? Or it is enough to send the *.diff to the mailing list assuming some of maintainers will review it and will push it into the version?
Comment 4 Daniel Veillard 2009-08-21 22:02:10 UTC
seeing comment #3 today, next release is within a few days, I would take
a VxWork folder if this look clean, a git patch would be fine, either on a
new bug or sent to the mailing-list

Daniel
Comment 5 Igor 2009-08-23 11:10:59 UTC
Failed to send response to the mail list, so reuse the bugzilla again ...

I was not clean, probably.
I don’t have a VxWorks folder. I saw this folder in the in the on-line project tree, referenced by libxml2-2.6.32.
I could propose my own Makefile.vxworks file, that assumes CYGWIN and Windriver Workbench are installed.
But to my sorrow, I have no time now to get it be ready for check-in into the closest release.
Therefore let's postpone the issue for the future releases.

Thank you,
Igor
Comment 6 Daniel Veillard 2009-08-24 12:05:15 UTC
  Okay, thanks,
I will try to fix the build so the VxWorks folder is in the tarball
that's trivial I think it's just an oversight,

Daniel
Comment 7 Kevin Puetz 2010-12-03 18:28:03 UTC
Created attachment 175800 [details] [review]
Move initialization of cleanup_helpers_cs to xmlOnceInit
Comment 8 Kevin Puetz 2010-12-03 18:30:51 UTC
I just rediscovered this bug (in a project that was also using 2.7.3; I'm not saying the fix didn't work), but I independently tracked it down and came up with a different fix before finding this bugzilla. I think waht I did might eb a little clearer, so I'll post it and see if you and Igor agree :-)

The logical flaw in what Igor did is that xmlInitThreads() is not threadsafely locked (there are checks, but no mutexes/memory barriers) against getting run more than once if threads race. This is OK except on win32, because what it does on other platforms is idempotent anyway. However, initializing the critical section is not, and needs to only happen once. I think in practice this is OK, because it has only two callers: xmlInitParser is safely locked, and xmlIsMainThread only calls xmlInitThreads on posix, where this critical section is #ifdef'ed out. But it's a little ugly. 

What I did instead of adding these calls to xmlInitParser like Igor did, was to move the critical section initialization from xmlInitThreads to xmlOnceInit, which does use interlocked operations for its guard variables.

This also fixes the bug Igor reported, because xmlOnceInit is run as necessary by xmlIsMainThread and xmlGetGlobalState; xmlInitThreads was not.