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 681467 - Global extra_parser_data is not thread safe
Global extra_parser_data is not thread safe
Status: RESOLVED FIXED
Product: libxml++
Classification: Bindings
Component: General
2.35.x
Other All
: Normal normal
: ---
Assigned To: Christophe de Vienne
Christophe de Vienne
Depends on:
Blocks:
 
 
Reported: 2012-08-08 16:55 UTC by Iwan Aucamp
Modified: 2012-08-28 14:22 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
patch: Parser: Make it thread-safe. (3.96 KB, patch)
2012-08-09 14:43 UTC, Kjell Ahlstedt 		none Details | Review

Description Iwan Aucamp 2012-08-08 16:55:43 UTC 
In libxml++/parsers/parser.cc extra_parser_data (a map of structs) is used as interim measure to prevent ABI breakage - but there is no synchronization around it - this will lead to problems if multiple threads work with multiple Parser objects.
Comment 1 Kjell Ahlstedt 2012-08-09 14:43:26 UTC 
Created attachment 220792 [details] [review]
patch: Parser: Make it thread-safe.

You're right. I added extra_parser_data when I fixed bug 304020, but
I didn't think of multi-threaded programs. Thanks for noticing it!
Luckily my incomplete fix has not yet been included in a stable release of
libxml++.

The attached patch fixes the problem, I hope. I have two questions, though,
that I hope someone can answer.

1. I use a Glib::Threads::Mutex for synchronization. Is that safe in a program
   that also use libxml2, which has its own thread handling?
   I suppose it is. Both libxml2 and Glib must ultimately rely on the operating
   system's thread support.

2. After this change, libxml++ requires glibmm version 2.32.0 or higher.
   That's the first version with Glib::Threads::Mutex.
   Is it acceptable to require glibmm-2.4 >= 2.32.0? Was glibmm-2.4 >= 2.4.0.

It would be possible to use an xmlMutex, but it's not as easy.
Comment 2 Murray Cumming 2012-08-10 02:04:04 UTC 
Wouldn't it be just as much of a problem even if these were regular static member variables? I just want to be clear about what the real problem is.
Comment 3 Kjell Ahlstedt 2012-08-10 08:07:25 UTC 
They wouldn't be static. They would be instance member variables.

I added a local map in libxml++/parsers/parser.cc

  std::map<const xmlpp::Parser*, ExtraParserData> extra_parser_data;

with some new data for each created Parser. Each Parser instance uses only its
own data in this map by accessing extra_parser_data[this]. There will be
synchronisation problems if a key is added or removed in the map in one thread
while the map is accessed in another thread. Perhaps in other situations, too.

The struct ExtraParserData contains some data that I want to add as instance
member data to xmlpp::Parser when we can break ABI. The std::map will not be
needed then.
Comment 4 Kjell Ahlstedt 2012-08-10 09:33:36 UTC 
Sorry, I don't think comment 3 answered your question in comment 2.

Yes, if the map would be a static member variable, the thread synchronization
problem would be the same as it is now, when the map is local to parser.cc.