* [PATCH] pcm: Fix shm initialization race-condition
@ 2016-08-22 16:04 Ismael Luceno
2016-08-22 16:06 ` Ismael Luceno
2016-08-22 16:11 ` Takashi Iwai
0 siblings, 2 replies; 7+ messages in thread
From: Ismael Luceno @ 2016-08-22 16:04 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Ismael Luceno
Easily seen when two threads try at the same time, one of them will fail.
The bug was identified by using apulse with Skype.
Fixes: dec428c35221 ("pcm: fix 'unable to create IPC shm instance' caused by fork from a thread")
Fixes: https://github.com/i-rinat/apulse/issues/38
Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
---
src/pcm/pcm_direct.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index c3925cc20fd3..643498375b34 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -96,11 +96,12 @@ int snd_pcm_direct_shm_create_or_connect(snd_pcm_direct_t *dmix)
retryget:
dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
dmix->ipc_perm);
- if (dmix->shmid < 0) {
- if (errno == ENOENT)
+ if (dmix->shmid < 0 && errno == ENOENT) {
if ((dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
IPC_CREAT | IPC_EXCL | dmix->ipc_perm)) != -1)
first_instance = 1;
+ else if (errno == EEXIST)
+ goto retryget;
}
err = -errno;
if (dmix->shmid < 0) {
--
2.9.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm: Fix shm initialization race-condition
2016-08-22 16:04 [PATCH] pcm: Fix shm initialization race-condition Ismael Luceno
@ 2016-08-22 16:06 ` Ismael Luceno
2016-08-22 16:11 ` Takashi Iwai
1 sibling, 0 replies; 7+ messages in thread
From: Ismael Luceno @ 2016-08-22 16:06 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
On 22/Ago/2016 13:04, Ismael Luceno wrote:
> Easily seen when two threads try at the same time, one of them will fail.
>
> The bug was identified by using apulse with Skype.
>
> Fixes: dec428c35221 ("pcm: fix 'unable to create IPC shm instance' caused by fork from a thread")
> Fixes: https://github.com/i-rinat/apulse/issues/38
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
<...>
Oops, subject should be: [PATCH v2]...
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm: Fix shm initialization race-condition
2016-08-22 16:04 [PATCH] pcm: Fix shm initialization race-condition Ismael Luceno
2016-08-22 16:06 ` Ismael Luceno
@ 2016-08-22 16:11 ` Takashi Iwai
1 sibling, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2016-08-22 16:11 UTC (permalink / raw)
To: Ismael Luceno; +Cc: alsa-devel
On Mon, 22 Aug 2016 18:04:33 +0200,
Ismael Luceno wrote:
>
> Easily seen when two threads try at the same time, one of them will fail.
>
> The bug was identified by using apulse with Skype.
>
> Fixes: dec428c35221 ("pcm: fix 'unable to create IPC shm instance' caused by fork from a thread")
> Fixes: https://github.com/i-rinat/apulse/issues/38
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
OK, I'll apply this one, as this change alone is fine.
But in general, this implies that the application hitting this is
buggy. You can't reliably call snd_pcm_open() for the same PCM stream
concurrently via multi threads. You have to introduce the proper
protection in the caller side.
thanks,
Takashi
> ---
> src/pcm/pcm_direct.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> index c3925cc20fd3..643498375b34 100644
> --- a/src/pcm/pcm_direct.c
> +++ b/src/pcm/pcm_direct.c
> @@ -96,11 +96,12 @@ int snd_pcm_direct_shm_create_or_connect(snd_pcm_direct_t *dmix)
> retryget:
> dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
> dmix->ipc_perm);
> - if (dmix->shmid < 0) {
> - if (errno == ENOENT)
> + if (dmix->shmid < 0 && errno == ENOENT) {
> if ((dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
> IPC_CREAT | IPC_EXCL | dmix->ipc_perm)) != -1)
> first_instance = 1;
> + else if (errno == EEXIST)
> + goto retryget;
> }
> err = -errno;
> if (dmix->shmid < 0) {
> --
> 2.9.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm: Fix shm initialization race-condition
2016-08-22 15:11 ` Takashi Iwai
@ 2016-08-22 16:02 ` Ismael Luceno
0 siblings, 0 replies; 7+ messages in thread
From: Ismael Luceno @ 2016-08-22 16:02 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
On 22/Ago/2016 17:11, Takashi Iwai wrote:
> On Mon, 22 Aug 2016 17:02:41 +0200,
> Ismael Luceno wrote:
> >
> > On 22/Ago/2016 11:26, Takashi Iwai wrote:
> > > On Sun, 14 Aug 2016 02:28:52 +0200,
> > > Ismael Luceno wrote:
> > > >
> > > > Easily seen when two threads try at the same time, one of them will fail.
> > > >
> > > > The bug was identified by using apulse with Skype.
> > > >
> > > > Fixes: dec428c35221 ("pcm: fix 'unable to create IPC shm instance' caused by fork from a thread")
> > > > Fixes: https://github.com/i-rinat/apulse/issues/38
> > > > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> > > > ---
> > > > src/pcm/pcm_direct.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> > > > index c3925cc20fd3..b5215ba35406 100644
> > > > --- a/src/pcm/pcm_direct.c
> > > > +++ b/src/pcm/pcm_direct.c
> > > > @@ -101,6 +101,8 @@ retryget:
> > > > if ((dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
> > > > IPC_CREAT | IPC_EXCL | dmix->ipc_perm)) != -1)
> > > > first_instance = 1;
> > > > + if (dmix->shmid < 0 && errno == EEXIST)
> > > > + goto retryget;
> > >
> > > Hrm, but this would result in an endless loop if the shm was already
> > > taken persistently.
> >
> > If so, shouldn't the first call to shmget succeed?
> >
> > To me it seems very unlikely that both calls continuosly fail.
>
> Well, you are inserting a loop at the code below:
>
> retryget:
> dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
> dmix->ipc_perm);
> if (dmix->shmid < 0) {
> if (errno == ENOENT)
> if ((dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
> IPC_CREAT | IPC_EXCL | dmix->ipc_perm)) != -1)
> first_instance = 1;
> ==> if (dmix->shmid < 0 && errno == EEXIST)
> ==> goto retryget;
> }
>
>
> It's in the if block when the first shmget() fails. If there was
> already a shm with the given id that had been assigned by another (not
> necessarily by alsa-lib but by whatever program), the first shmget
> returns an error with EEXIST. Then it goes back again in a loop; and
> it can be endless if another program doesn't release the shm.
I will fix my patch, I see a problem now, but the spirit of the
idea should be right: if the first call fails with ENOENT, and the
second with EEXIST, it should retry, and getting EEXIST again should
be very unlikely.
> > > Also, which call does give a negative shmid, actually? It's from the
> > > first shmget() or the second shmget()?
> >
> > What happens is that both threads go down that path but, of course,
> > only one succeeds in the second shmget call.
>
> This should have been protected by a sempahore beforehand.
> And if it's about threads, the application itself has to take care of
> the race. alsa-lib is no thread-safe, after all.
>
> So, did you see the issue with multiple processes, or is it about the
> multi-threads?
Threads in this case, but could happen with processes as well.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm: Fix shm initialization race-condition
2016-08-22 15:02 ` Ismael Luceno
@ 2016-08-22 15:11 ` Takashi Iwai
2016-08-22 16:02 ` Ismael Luceno
0 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2016-08-22 15:11 UTC (permalink / raw)
To: Ismael Luceno; +Cc: alsa-devel
On Mon, 22 Aug 2016 17:02:41 +0200,
Ismael Luceno wrote:
>
> On 22/Ago/2016 11:26, Takashi Iwai wrote:
> > On Sun, 14 Aug 2016 02:28:52 +0200,
> > Ismael Luceno wrote:
> > >
> > > Easily seen when two threads try at the same time, one of them will fail.
> > >
> > > The bug was identified by using apulse with Skype.
> > >
> > > Fixes: dec428c35221 ("pcm: fix 'unable to create IPC shm instance' caused by fork from a thread")
> > > Fixes: https://github.com/i-rinat/apulse/issues/38
> > > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> > > ---
> > > src/pcm/pcm_direct.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> > > index c3925cc20fd3..b5215ba35406 100644
> > > --- a/src/pcm/pcm_direct.c
> > > +++ b/src/pcm/pcm_direct.c
> > > @@ -101,6 +101,8 @@ retryget:
> > > if ((dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
> > > IPC_CREAT | IPC_EXCL | dmix->ipc_perm)) != -1)
> > > first_instance = 1;
> > > + if (dmix->shmid < 0 && errno == EEXIST)
> > > + goto retryget;
> >
> > Hrm, but this would result in an endless loop if the shm was already
> > taken persistently.
>
> If so, shouldn't the first call to shmget succeed?
>
> To me it seems very unlikely that both calls continuosly fail.
Well, you are inserting a loop at the code below:
retryget:
dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
dmix->ipc_perm);
if (dmix->shmid < 0) {
if (errno == ENOENT)
if ((dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
IPC_CREAT | IPC_EXCL | dmix->ipc_perm)) != -1)
first_instance = 1;
==> if (dmix->shmid < 0 && errno == EEXIST)
==> goto retryget;
}
It's in the if block when the first shmget() fails. If there was
already a shm with the given id that had been assigned by another (not
necessarily by alsa-lib but by whatever program), the first shmget
returns an error with EEXIST. Then it goes back again in a loop; and
it can be endless if another program doesn't release the shm.
> > Also, which call does give a negative shmid, actually? It's from the
> > first shmget() or the second shmget()?
>
> What happens is that both threads go down that path but, of course,
> only one succeeds in the second shmget call.
This should have been protected by a sempahore beforehand.
And if it's about threads, the application itself has to take care of
the race. alsa-lib is no thread-safe, after all.
So, did you see the issue with multiple processes, or is it about the
multi-threads?
Takashi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm: Fix shm initialization race-condition
2016-08-22 9:26 ` Takashi Iwai
@ 2016-08-22 15:02 ` Ismael Luceno
2016-08-22 15:11 ` Takashi Iwai
0 siblings, 1 reply; 7+ messages in thread
From: Ismael Luceno @ 2016-08-22 15:02 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
On 22/Ago/2016 11:26, Takashi Iwai wrote:
> On Sun, 14 Aug 2016 02:28:52 +0200,
> Ismael Luceno wrote:
> >
> > Easily seen when two threads try at the same time, one of them will fail.
> >
> > The bug was identified by using apulse with Skype.
> >
> > Fixes: dec428c35221 ("pcm: fix 'unable to create IPC shm instance' caused by fork from a thread")
> > Fixes: https://github.com/i-rinat/apulse/issues/38
> > Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> > ---
> > src/pcm/pcm_direct.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> > index c3925cc20fd3..b5215ba35406 100644
> > --- a/src/pcm/pcm_direct.c
> > +++ b/src/pcm/pcm_direct.c
> > @@ -101,6 +101,8 @@ retryget:
> > if ((dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
> > IPC_CREAT | IPC_EXCL | dmix->ipc_perm)) != -1)
> > first_instance = 1;
> > + if (dmix->shmid < 0 && errno == EEXIST)
> > + goto retryget;
>
> Hrm, but this would result in an endless loop if the shm was already
> taken persistently.
If so, shouldn't the first call to shmget succeed?
To me it seems very unlikely that both calls continuosly fail.
> Also, which call does give a negative shmid, actually? It's from the
> first shmget() or the second shmget()?
What happens is that both threads go down that path but, of course,
only one succeeds in the second shmget call.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm: Fix shm initialization race-condition
[not found] <20160814002852.4013-1-ismael@iodev.co.uk>
@ 2016-08-22 9:26 ` Takashi Iwai
2016-08-22 15:02 ` Ismael Luceno
0 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2016-08-22 9:26 UTC (permalink / raw)
To: Ismael Luceno; +Cc: alsa-devel
On Sun, 14 Aug 2016 02:28:52 +0200,
Ismael Luceno wrote:
>
> Easily seen when two threads try at the same time, one of them will fail.
>
> The bug was identified by using apulse with Skype.
>
> Fixes: dec428c35221 ("pcm: fix 'unable to create IPC shm instance' caused by fork from a thread")
> Fixes: https://github.com/i-rinat/apulse/issues/38
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> ---
> src/pcm/pcm_direct.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> index c3925cc20fd3..b5215ba35406 100644
> --- a/src/pcm/pcm_direct.c
> +++ b/src/pcm/pcm_direct.c
> @@ -101,6 +101,8 @@ retryget:
> if ((dmix->shmid = shmget(dmix->ipc_key, sizeof(snd_pcm_direct_share_t),
> IPC_CREAT | IPC_EXCL | dmix->ipc_perm)) != -1)
> first_instance = 1;
> + if (dmix->shmid < 0 && errno == EEXIST)
> + goto retryget;
Hrm, but this would result in an endless loop if the shm was already
taken persistently.
Also, which call does give a negative shmid, actually? It's from the
first shmget() or the second shmget()?
(The existing code is ugly and we need to fix the indentation
there...)
thanks,
Takashi
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-22 16:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-22 16:04 [PATCH] pcm: Fix shm initialization race-condition Ismael Luceno
2016-08-22 16:06 ` Ismael Luceno
2016-08-22 16:11 ` Takashi Iwai
[not found] <20160814002852.4013-1-ismael@iodev.co.uk>
2016-08-22 9:26 ` Takashi Iwai
2016-08-22 15:02 ` Ismael Luceno
2016-08-22 15:11 ` Takashi Iwai
2016-08-22 16:02 ` Ismael Luceno
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.