[alsa-devel] [PATCH] alsa-lib: Protect from freeing semaphore when already in use

[Takashi Iwai]

On Fri, 25 Nov 2016 11:13:40 +0100,
sutar.mounesh at gmail.com wrote:
> 
> From: Joshua Frkuska <joshua_frkuska at mentor.com>
> 
> In the case of dshare, dsnoop, and dmix when a device is opened twice
> and fails the second time, the semaphore is completely discarded. This
> creates dangling semaphore data.
> 
> This patch removes the possibility for the semaphore to be destroyed during
> a typical open failure by first checking if the shared memory can be destroyed
> or not. If the shared memory cannot be released it means both it and the
> semaphore are still in use and therefore the semaphore is just released.
> 
> Signed-off-by: Joshua Frkuska <joshua_frkuska at mentor.com>

Do you have some test code?  It'd be great for checking the bug and
avoiding regressions.

In anyway, the changes look good to me, so I applied it now.


thanks,

Takashi


> ---
>  src/pcm/pcm_dmix.c   |    7 ++++---
>  src/pcm/pcm_dshare.c |    7 ++++---
>  src/pcm/pcm_dsnoop.c |    8 +++++---
>  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
> index 2bd5d39..b17f759 100644
> --- a/src/pcm/pcm_dmix.c
> +++ b/src/pcm/pcm_dmix.c
> @@ -1128,9 +1128,10 @@ int snd_pcm_dmix_open(snd_pcm_t **pcmp, const char *name,
>  		snd_pcm_close(spcm);
>  	if (dmix->u.dmix.shmid_sum >= 0)
>  		shm_sum_discard(dmix);
> -	if (dmix->shmid >= 0)
> -		snd_pcm_direct_shm_discard(dmix);
> -	if (snd_pcm_direct_semaphore_discard(dmix) < 0)
> +	if ((dmix->shmid >= 0) && (snd_pcm_direct_shm_discard(dmix))) {
> +		if (snd_pcm_direct_semaphore_discard(dmix))
> +			snd_pcm_direct_semaphore_final(dmix, DIRECT_IPC_SEM_CLIENT);
> +	} else
>  		snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
>   _err_nosem:
>  	if (dmix) {
> diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
> index 5b32951..c080e7a 100644
> --- a/src/pcm/pcm_dshare.c
> +++ b/src/pcm/pcm_dshare.c
> @@ -813,9 +813,10 @@ int snd_pcm_dshare_open(snd_pcm_t **pcmp, const char *name,
>  		snd_pcm_direct_client_discard(dshare);
>  	if (spcm)
>  		snd_pcm_close(spcm);
> -	if (dshare->shmid >= 0)
> -		snd_pcm_direct_shm_discard(dshare);
> -	if (snd_pcm_direct_semaphore_discard(dshare) < 0)
> +	if ((dshare->shmid >= 0) && (snd_pcm_direct_shm_discard(dshare))) {
> +		if (snd_pcm_direct_semaphore_discard(dshare))
> +			snd_pcm_direct_semaphore_final(dshare, DIRECT_IPC_SEM_CLIENT);
> +	} else
>  		snd_pcm_direct_semaphore_up(dshare, DIRECT_IPC_SEM_CLIENT);
>   _err_nosem:
>  	if (dshare) {
> diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
> index 055e4f4..74a3531 100644
> --- a/src/pcm/pcm_dsnoop.c
> +++ b/src/pcm/pcm_dsnoop.c
> @@ -712,10 +712,12 @@ int snd_pcm_dsnoop_open(snd_pcm_t **pcmp, const char *name,
>  		snd_pcm_direct_client_discard(dsnoop);
>  	if (spcm)
>  		snd_pcm_close(spcm);
> -	if (dsnoop->shmid >= 0)
> -		snd_pcm_direct_shm_discard(dsnoop);
> -	if (snd_pcm_direct_semaphore_discard(dsnoop) < 0)
> +	if ((dsnoop->shmid >= 0) && (snd_pcm_direct_shm_discard(dsnoop))) {
> +		if (snd_pcm_direct_semaphore_discard(dsnoop))
> +			snd_pcm_direct_semaphore_final(dsnoop, DIRECT_IPC_SEM_CLIENT);
> +	} else
>  		snd_pcm_direct_semaphore_up(dsnoop, DIRECT_IPC_SEM_CLIENT);
> +
>   _err_nosem:
>  	if (dsnoop) {
>  		free(dsnoop->bindings);
> -- 
> 1.7.9.5
>