[libvirt] Potential race condition problem
Hi, Currently virInitialize() method defined in libvirt.c has the following code: int virInitialize(void) { if (initialized) return 0; initialized = 1; if (virThreadInitialize() < 0 || virErrorInitialize() < 0 || virRandomInitialize(time(NULL) ^ getpid()) || virNodeSuspendInit() < 0) return -1; ...... } When two threads access virInitialize method, there is no lock for the "initialized" parameter. If the first thread enters this method and set "initialized" to 1, the second thread could see that "initialized" is 1(Because initialized is not volatiled, I say could). In some situation, before the first thread finishes all the initialization, the second thread could use some resources which should be initialized in Initialize method. If you have any comments, please let me know. Thanks! B.R. Benjamin Wang
On 09/29/2012 03:07 PM, Benjamin Wang (gendwang) wrote:
Hi,
Currently virInitialize() method defined in libvirt.c has the following code:
int
virInitialize(void)
{
if (initialized)
return 0;
initialized = 1;
if (virThreadInitialize() < 0 ||
virErrorInitialize() < 0 ||
virRandomInitialize(time(NULL) ^ getpid()) ||
virNodeSuspendInit() < 0)
return -1;
......
}
When two threads access virInitialize method, there is no lock for the "initialized" parameter. If the first thread enters this method and set "initialized" to 1,
the second thread could see that "initialized" is 1(Because initialized is not volatiled, I say could). In some situation, before the first thread finishes all the initialization,
the second thread could use some resources which should be initialized in Initialize method.
If you have any comments, please let me know. Thanks!
B.R.
Benjamin Wang
As the comments above the function said, "It's better to call this routine at startup in multithreaded applications to avoid potential race when initializing the library." Guannan
Hi, OK. Now I am using JNA to access libvirt. If we add another mutex which used to access “initialized” parameter. This mutex must be pthread_mutex_init firstly and only once. But it seems that there is no way to change libvirt code. I do it as following: 1. Changing libvirt JNA code in Connect.java Old Code: public Connect(String uri) throws LibvirtException { VCP = libvirt.virConnectOpen(uri); // Check for an error processError(); ErrorHandler.processError(Libvirt.INSTANCE); } New Code: public Connect(String uri) throws LibvirtException { synchronized(this.getClass()) { VCP = libvirt.virConnectOpen(uri); } // Check for an error processError(); ErrorHandler.processError(Libvirt.INSTANCE); } This can make sure only that one thread can execute Connect. For a server application, we only need one time. So the performance is OK 2. Changing libvirt code in libvirt.c Old Code: static int initialized = 0; New Code: static int volatile initialized = 0; This can make sure the initialization will be executed once. Would you give your comments for this solution? B.R. Benjamin Wang From: Guannan Ren [mailto:gren@redhat.com] Sent: 2012年9月29日 15:43 To: Benjamin Wang (gendwang) Cc: Daniel Veillard; libvir-list@redhat.com; Yang Zhou (yangzho) Subject: Re: [libvirt] Potential race condition problem On 09/29/2012 03:07 PM, Benjamin Wang (gendwang) wrote: Hi, Currently virInitialize() method defined in libvirt.c has the following code: int virInitialize(void) { if (initialized) return 0; initialized = 1; if (virThreadInitialize() < 0 || virErrorInitialize() < 0 || virRandomInitialize(time(NULL) ^ getpid()) || virNodeSuspendInit() < 0) return -1; …… } When two threads access virInitialize method, there is no lock for the “initialized” parameter. If the first thread enters this method and set “initialized” to 1, the second thread could see that “initialized” is 1(Because initialized is not volatiled, I say could). In some situation, before the first thread finishes all the initialization, the second thread could use some resources which should be initialized in Initialize method. If you have any comments, please let me know. Thanks! B.R. Benjamin Wang As the comments above the function said, "It's better to call this routine at startup in multithreaded applications to avoid potential race when initializing the library." Guannan
On 09/29/2012 03:52 PM, Benjamin Wang (gendwang) wrote:
Hi,
OK. Now I am using JNA to access libvirt. If we add another mutex which used to access “initialized” parameter. This mutex must be pthread_mutex_init firstly and only once.
But it seems that there is no way to change libvirt code. I do it as following:
1.Changing libvirt JNA code in Connect.java
Old Code:
public Connect(String uri) throws LibvirtException {
VCP = libvirt.virConnectOpen(uri);
// Check for an error
processError();
ErrorHandler.processError(Libvirt.INSTANCE);
}
New Code:
public Connect(String uri) throws LibvirtException {
synchronized(this.getClass()) {
VCP = libvirt.virConnectOpen(uri);
}
// Check for an error
processError();
ErrorHandler.processError(Libvirt.INSTANCE);
}
This can make sure only that one thread can execute Connect. For a server application, we only need one time. So the performance is OK
2.Changing libvirt code in libvirt.c
Old Code:
static int initialized = 0;
New Code:
static int volatile initialized = 0;
This can make sure the initialization will be executed once.
Would you give your comments for this solution?
B.R.
Benjamin Wang
As far as I know the operations on |volatile| variable is not atomic, the usage of |volatile| keyword as a portable synchronization mechanism is discouraged by C. But in Java, it is a global ordering on the reads and writes to a volatile variable. So, maybe, your first solution is pretty enough good. Guannan
Hi, I think you misunderstand my meaning. My solution includes step1 + step2. Step1 is used to implement thread mutex. Step2 is used to handle “initialized” visibility. Without step2, the initialization could be executed several times. B.R. Benjamin Wang From: Guannan Ren [mailto:gren@redhat.com] Sent: 2012年9月29日 17:22 To: Benjamin Wang (gendwang) Cc: Daniel Veillard; libvir-list@redhat.com; Yang Zhou (yangzho); cbley@av-test.de Subject: Re: [libvirt] Potential race condition problem On 09/29/2012 03:52 PM, Benjamin Wang (gendwang) wrote: Hi, OK. Now I am using JNA to access libvirt. If we add another mutex which used to access “initialized” parameter. This mutex must be pthread_mutex_init firstly and only once. But it seems that there is no way to change libvirt code. I do it as following: 1. Changing libvirt JNA code in Connect.java Old Code: public Connect(String uri) throws LibvirtException { VCP = libvirt.virConnectOpen(uri); // Check for an error processError(); ErrorHandler.processError(Libvirt.INSTANCE); } New Code: public Connect(String uri) throws LibvirtException { synchronized(this.getClass()) { VCP = libvirt.virConnectOpen(uri); } // Check for an error processError(); ErrorHandler.processError(Libvirt.INSTANCE); } This can make sure only that one thread can execute Connect. For a server application, we only need one time. So the performance is OK 2. Changing libvirt code in libvirt.c Old Code: static int initialized = 0; New Code: static int volatile initialized = 0; This can make sure the initialization will be executed once. Would you give your comments for this solution? B.R. Benjamin Wang As far as I know the operations on volatile variable is not atomic, the usage of volatile keyword as a portable synchronization mechanism is discouraged by C. But in Java, it is a global ordering on the reads and writes to a volatile variable. So, maybe, your first solution is pretty enough good. Guannan
On 09/29/2012 09:06 PM, Benjamin Wang (gendwang) wrote:
Hi,
I think you misunderstand my meaning. My solution includes step1 + step2. Step1 is used to implement thread mutex. Step2 is used to
handle “initialized” visibility. Without step2, the initialization could be executed several times.
B.R.
Benjamin Wang
Okay, your solution probably work in Java because the visibility feature of "volatile", that is to say A |volatile| variable is not allowed to have a local copy of a variable in thread-local memory that is different from the value currently held in "main" memory. whenever you access or update the variable in any thread, all other threads immediately see the same value. But in C I am not quite sure that volatile will work like what it does in Java or it could work well with "synchronized" code in Java.
Hi, At Sat, 29 Sep 2012 07:52:42 +0000, Benjamin Wang (gendwang) wrote:
Hi,
OK. Now I am using JNA to access libvirt.
Actually, there is no problem in Java as virInitialize is called once in a static initializer (in a thread-safe manner) when the class gets first loaded by the class-loader. -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
On Sat, Sep 29, 2012 at 07:07:15AM +0000, Benjamin Wang (gendwang) wrote:
Hi, Currently virInitialize() method defined in libvirt.c has the following code: int virInitialize(void) { if (initialized) return 0;
initialized = 1;
if (virThreadInitialize() < 0 || virErrorInitialize() < 0 || virRandomInitialize(time(NULL) ^ getpid()) || virNodeSuspendInit() < 0) return -1;
...... }
When two threads access virInitialize method, there is no lock for the "initialized" parameter. If the first thread enters this method and set "initialized" to 1, the second thread could see that "initialized" is 1(Because initialized is not volatiled, I say could). In some situation, before the first thread finishes all the initialization, the second thread could use some resources which should be initialized in Initialize method. If you have any comments, please let me know. Thanks!
The API docs tell you to call this method upfront to avoid race conditions with multiple threads. So if you have multiple threads calling virInitialize in parallel, that is an application bug. That said, we could switch virInitilize to use our new one-time init support to remove this restriction in future libvirt. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Benjamin Wang (gendwang) -
Claudio Bley -
Daniel P. Berrange -
Guannan Ren