Re: alsa-lib patches

Re: alsa-lib patches

[Kevin F.Quinn]

Clemens Ladisch wrote:
> Martin Stransky wrote:
> > About mixer patch - some user had a problem with ICH6 and this patch helped.
> >
> > +static snd_mixer_t *qsort_mixer;
>
> This will break with multithreading.
>
> > +static int compar(const void *a, const void *b) {
> > +       return qsort_mixer->compare(*(const snd_mixer_elem_t * const *) a,
> > +                                   *(const snd_mixer_elem_t * const *) b);
> > +}
>
> So the actual problem is that qsort() calls us with a pointer to a
> pointer to the mixer_elem_t, but the compare function takes a single
> pointer.
>
>
> If we don"t want to change the mixer API, we have to use some
> pthread_mutex to protext qsort_mixer.

Some of us have been watching this at Gentoo, as we agree the patch applied to remove the nested function in src/mixer/mixer.c is broken.

See https://bugs.gentoo.org/show_bug.cgi?id=82242 (skip straight to the end, comment #13) where the PaX team have supplied a patch that ought to work, along the lines of a change made to src/control/hcontrol.c over a year ago which fixed the same issue there.  Patch at https://bugs.gentoo.org/attachment.cgi?id=58918

Cheers,
Kev.




-------------------------------------------------------
This SF.Net email is sponsored by Oracle Space Sweepstakes
Want to be the first software developer in space?
Enter now for the Oracle Space Sweepstakes!
http://ads.osdn.com/?ad_ids93&alloc_id\x16281&op=click

Re: Re: alsa-lib patches

[James Courtier-Dutton]

Kevin F.Quinn wrote:
> Clemens Ladisch wrote:
> 
>>Martin Stransky wrote:
>>
>>>About mixer patch - some user had a problem with ICH6 and this patch helped.
>>>
>>>+static snd_mixer_t *qsort_mixer;
>>
>> 
>>This will break with multithreading.
>> 
>>
>>>+static int compar(const void *a, const void *b) {
>>>+       return qsort_mixer->compare(*(const snd_mixer_elem_t * const *) a,
>>>+                                   *(const snd_mixer_elem_t * const *) b);
>>>+}
>>
>> 
>>So the actual problem is that qsort() calls us with a pointer to a
>>pointer to the mixer_elem_t, but the compare function takes a single
>>pointer.
>> 
>> 
>>If we don"t want to change the mixer API, we have to use some
>>pthread_mutex to protext qsort_mixer.
> 
> 
> Some of us have been watching this at Gentoo, as we agree the patch applied to remove the nested function in src/mixer/mixer.c is broken.
> 
> See https://bugs.gentoo.org/show_bug.cgi?id=82242 (skip straight to the end, comment #13) where the PaX team have supplied a patch that ought to work, along the lines of a change made to src/control/hcontrol.c over a year ago which fixed the same issue there.  Patch at https://bugs.gentoo.org/attachment.cgi?id=58918
> 
> Cheers,
> Kev.
> 

What is wrong with the current CVS code?:

typedef int (*qsort_func)(const void *, const void *);
static int snd_mixer_sort(snd_mixer_t *mixer)
{
        unsigned int k;
        assert(mixer);
        assert(mixer->compare);
        INIT_LIST_HEAD(&mixer->elems);
        qsort(mixer->pelems, mixer->count, sizeof(snd_mixer_elem_t*),
(qsort_func)mixer->compare);
        for (k = 0; k < mixer->count; k++)
                list_add_tail(&mixer->pelems[k]->list, &mixer->elems);
        return 0;
}

We are not executing any code on the stack. This should not effect the
NX bit so what I the problem here?

James


-------------------------------------------------------
This SF.Net email is sponsored by Oracle Space Sweepstakes
Want to be the first software developer in space?
Enter now for the Oracle Space Sweepstakes!
http://ads.osdn.com/?ad_id=7393&alloc_id=16281&op=click

Re: Re: alsa-lib patches

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

On Sun, May 15, 2005 at 06:30:40PM +0100, James Courtier-Dutton wrote:
> What is wrong with the current CVS code?:
> 
> typedef int (*qsort_func)(const void *, const void *);
> static int snd_mixer_sort(snd_mixer_t *mixer)
> {
>         unsigned int k;
>         assert(mixer);
>         assert(mixer->compare);
>         INIT_LIST_HEAD(&mixer->elems);
>         qsort(mixer->pelems, mixer->count, sizeof(snd_mixer_elem_t*),
> (qsort_func)mixer->compare);
>         for (k = 0; k < mixer->count; k++)
>                 list_add_tail(&mixer->pelems[k]->list, &mixer->elems);
>         return 0;
> }
> 
> We are not executing any code on the stack. This should not effect the
> NX bit so what I the problem here?

The problem was cited above:

> >>So the actual problem is that qsort() calls us with a pointer to a
> >>pointer to the mixer_elem_t, but the compare function takes a single
> >>pointer.

mixer->pelems is an array of pointers to snd_mixer_elem_t, and qsort()
passes pointers to array elements as arguments to the compare
function - which becomes (snd_mixer_elem_t **).  But mixer->compare
expects to get (snd_mixer_elem_t *) as arguments.

However, seems that mixer elements have an indirect pointer to the
mixer they belong to - elem->class->mixer.  So, how about this:

static int snd_mixer_qsort_compare(const void *a, const void *b)
{
	snd_mixer_elem_t *elem_a = *(snd_mixer_elem_t **)a;
	snd_mixer_elem_t *elem_b = *(snd_mixer_elem_t **)b;

	return elem_a->class->mixer->compare(elem_a, elem_b);
}

static int snd_mixer_sort(snd_mixer_t *mixer)
{
	unsigned int k;
	assert(mixer);
	assert(mixer->compare);
	INIT_LIST_HEAD(&mixer->elems);
	qsort(mixer->pelems, mixer->count, sizeof(snd_mixer_elem_t*), snd_mixer_qsort_compare);
	for (k = 0; k < mixer->count; k++)
		list_add_tail(&mixer->pelems[k]->list, &mixer->elems);
	return 0;
}

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Re: alsa-lib patches

[James Courtier-Dutton]

Sergey Vlasov wrote:
> 
> mixer->pelems is an array of pointers to snd_mixer_elem_t, and qsort()
> passes pointers to array elements as arguments to the compare
> function - which becomes (snd_mixer_elem_t **).  But mixer->compare
> expects to get (snd_mixer_elem_t *) as arguments.
> 
Ok, so not a security issue, just a messed up pointer issue.


> However, seems that mixer elements have an indirect pointer to the
> mixer they belong to - elem->class->mixer.  So, how about this:
> 
> static int snd_mixer_qsort_compare(const void *a, const void *b)
> {
> 	snd_mixer_elem_t *elem_a = *(snd_mixer_elem_t **)a;
> 	snd_mixer_elem_t *elem_b = *(snd_mixer_elem_t **)b;
> 
> 	return elem_a->class->mixer->compare(elem_a, elem_b);
> }
> 
> static int snd_mixer_sort(snd_mixer_t *mixer)
> {
> 	unsigned int k;
> 	assert(mixer);
> 	assert(mixer->compare);
> 	INIT_LIST_HEAD(&mixer->elems);
> 	qsort(mixer->pelems, mixer->count, sizeof(snd_mixer_elem_t*), snd_mixer_qsort_compare);
> 	for (k = 0; k < mixer->count; k++)
> 		list_add_tail(&mixer->pelems[k]->list, &mixer->elems);
> 	return 0;
> }

That seems like a better solution. This sort routine does not have to be
high performance as it is not used often.

I suppose this begs the question, why has it work ok up until now?

James




-------------------------------------------------------
This SF.Net email is sponsored by Oracle Space Sweepstakes
Want to be the first software developer in space?
Enter now for the Oracle Space Sweepstakes!
http://ads.osdn.com/?ad_id=7393&alloc_id=16281&op=click

Re: Re: alsa-lib patches

[Kevin F.Quinn]

On 15/5/2005 19:21:02, Sergey Vlasov (vsu@altlinux.ru) wrote:
> On Sun, May 15, 2005 at 06:30:40PM +0100, James Courtier-Dutton wrote:
> > What is wrong with the current CVS code?:
> >
> > typedef int (*qsort_func)(const void *, const void *);
> > static int snd_mixer_sort(snd_mixer_t *mixer)
> > {
> >         unsigned int k;
> >         assert(mixer);
> >         assert(mixer->compare);
> >         INIT_LIST_HEAD(&mixer->elems);
> >         qsort(mixer->pelems, mixer->count, sizeof(snd_mixer_elem_t*),
> > (qsort_func)mixer->compare);
> >         for (k = 0; k < mixer->count; k++)
> >                 list_add_tail(&mixer->pelems[k]->list, &mixer->elems);
> >         return 0;
> > }
> >
> > We are not executing any code on the stack. This should not effect the
> > NX bit so what I the problem here?
>
> The problem was cited above:
>
> > >>So the actual problem is that qsort() calls us with a pointer to a
> > >>pointer to the mixer_elem_t, but the compare function takes a single
> > >>pointer.
>
> mixer->pelems is an array of pointers to snd_mixer_elem_t, and qsort()
> passes pointers to array elements as arguments to the compare
> function - which becomes (snd_mixer_elem_t **).  But mixer->compare
> expects to get (snd_mixer_elem_t *) as arguments.
>
> However, seems that mixer elements have an indirect pointer to the
> mixer they belong to - elem->class->mixer.  So, how about this:
>
> static int snd_mixer_qsort_compare(const void *a, const void *b)
> {
> 	snd_mixer_elem_t *elem_a = *(snd_mixer_elem_t **)a;
> 	snd_mixer_elem_t *elem_b = *(snd_mixer_elem_t **)b;
>
> 	return elem_a->class->mixer->compare(elem_a, elem_b);
> }
>
> static int snd_mixer_sort(snd_mixer_t *mixer)
> {
> 	unsigned int k;
> 	assert(mixer);
> 	assert(mixer->compare);
> 	INIT_LIST_HEAD(&mixer->elems);
> 	qsort(mixer->pelems, mixer->count, sizeof(snd_mixer_elem_t*),
> snd_mixer_qsort_compare);
> 	for (k = 0; k < mixer->count; k++)
> 		list_add_tail(&mixer->pelems[k]->list, &mixer->elems);
> 	return 0;
> }

You also need to make it thread-safe, as Clements mentioned; the PaX team's proposal at https://bugs.gentoo.org/attachment.cgi?id=58918 does that as well:


--- alsa-lib-1.0.9rc3/src/mixer/mixer.c	2005-02-04 19:18:49.000000000 +0000
+++ alsa-lib-1.0.9rc3-fixed/src/mixer/mixer.c	2005-05-15 00:10:59.000000000 +0100
@@ -45,6 +45,7 @@ This is an abstraction layer over the hc
 #include <string.h>
 #include <fcntl.h>
 #include <sys/ioctl.h>
+#include <pthread.h>
 #include "mixer_local.h"

 #ifndef DOC_HIDDEN
@@ -520,14 +521,26 @@ static int snd_mixer_compare_default(con
 	return c1->class->compare(c1, c2);
 }

-typedef int (*qsort_func)(const void *, const void *);
+static snd_mixer_t *compare_mixer;
+static int mixer_compare(const void *a, const void *b) {
+	return compare_mixer->compare(*(const snd_mixer_elem_t * const *) a,
+				      *(const snd_mixer_elem_t * const *) b);
+}
+
 static int snd_mixer_sort(snd_mixer_t *mixer)
 {
 	unsigned int k;
+	static pthread_mutex_t sync_lock = PTHREAD_MUTEX_INITIALIZER;
+
 	assert(mixer);
 	assert(mixer->compare);
 	INIT_LIST_HEAD(&mixer->elems);
-	qsort(mixer->pelems, mixer->count, sizeof(snd_mixer_elem_t*), (qsort_func)mixer->compare);
+
+	pthread_mutex_lock(&sync_lock);
+	compare_mixer = mixer;
+	qsort(mixer->pelems, mixer->count, sizeof(snd_mixer_elem_t*), mixer_compare);
+	pthread_mutex_unlock(&sync_lock);
+
 	for (k = 0; k < mixer->count; k++)
 		list_add_tail(&mixer->pelems[k]->list, &mixer->elems);
 	return 0;




-------------------------------------------------------
This SF.Net email is sponsored by Oracle Space Sweepstakes
Want to be the first software developer in space?
Enter now for the Oracle Space Sweepstakes!
http://ads.osdn.com/?ad_ids93&alloc_id\x16281&op=click

Re: Re: alsa-lib patches

[Lee Revell]

On Sun, 2005-05-15 at 22:24 +0200, Kevin F.Quinn wrote:
> You also need to make it thread-safe, as Clements mentioned

Why?  The rest of alsa-lib is not thread-safe.

Lee



-------------------------------------------------------
This SF.Net email is sponsored by Oracle Space Sweepstakes
Want to be the first software developer in space?
Enter now for the Oracle Space Sweepstakes!
http://ads.osdn.com/?ad_id=7393&alloc_id=16281&op=click

Re: Re: alsa-lib patches

[Kevin F.Quinn]

On 15/5/2005 22:34:50, Lee Revell (rlrevell@joe-job.com) wrote:
> On Sun, 2005-05-15 at 22:24 +0200, Kevin F.Quinn wrote:
> > You also need to make it thread-safe, as Clements mentioned
>
> Why?  The rest of alsa-lib is not thread-safe.

Only by comparison with control/hcontrol.c, which has a similar situation already in that style (see snd_hctl_sort()); also conf.c, pcm/pcm_meter.c and pcm/pcm_share.c go to the trouble of pthread mutexes.  If it's overkill there, then fair enough.




-------------------------------------------------------
This SF.Net email is sponsored by Oracle Space Sweepstakes
Want to be the first software developer in space?
Enter now for the Oracle Space Sweepstakes!
http://ads.osdn.com/?ad_ids93&alloc_id\x16281&op=click

Re: Re: alsa-lib patches

[Lee Revell]

On Mon, 2005-05-16 at 00:15 +0200, Kevin F.Quinn wrote:
> On 15/5/2005 22:34:50, Lee Revell (rlrevell@joe-job.com) wrote:
> > On Sun, 2005-05-15 at 22:24 +0200, Kevin F.Quinn wrote:
> > > You also need to make it thread-safe, as Clements mentioned
> > 
> > Why?  The rest of alsa-lib is not thread-safe.
> 
> Only by comparison with control/hcontrol.c, which has a similar situation already in that style (see snd_hctl_sort()); also conf.c, pcm/pcm_meter.c and pcm/pcm_share.c go to the trouble of pthread mutexes.  If it's overkill there, then fair enough.
> 

OK, makes sense.

Lee



-------------------------------------------------------
This SF.Net email is sponsored by Oracle Space Sweepstakes
Want to be the first software developer in space?
Enter now for the Oracle Space Sweepstakes!
http://ads.osdn.com/?ad_id=7393&alloc_id=16281&op=click

Re: Re: alsa-lib patches

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]

On Sun, May 15, 2005 at 10:24:03PM +0200, Kevin F. Quinn wrote:
> On 15/5/2005 19:21:02, Sergey Vlasov (vsu@altlinux.ru) wrote:
[skip]
> > static int snd_mixer_qsort_compare(const void *a, const void *b)
> > {
> > 	snd_mixer_elem_t *elem_a = *(snd_mixer_elem_t **)a;
> > 	snd_mixer_elem_t *elem_b = *(snd_mixer_elem_t **)b;
> >
> > 	return elem_a->class->mixer->compare(elem_a, elem_b);
> > }
> >
> > static int snd_mixer_sort(snd_mixer_t *mixer)
> > {
> > 	unsigned int k;
> > 	assert(mixer);
> > 	assert(mixer->compare);
> > 	INIT_LIST_HEAD(&mixer->elems);
> > 	qsort(mixer->pelems, mixer->count, sizeof(snd_mixer_elem_t*),
> > snd_mixer_qsort_compare);
> > 	for (k = 0; k < mixer->count; k++)
> > 		list_add_tail(&mixer->pelems[k]->list, &mixer->elems);
> > 	return 0;
> > }
> 
> You also need to make it thread-safe, as Clements mentioned; the PaX
> team's proposal at https://bugs.gentoo.org/attachment.cgi?id=58918 does
> that as well:

It does that only because it uses a static variable:

> +static snd_mixer_t *compare_mixer;
> +static int mixer_compare(const void *a, const void *b) {
> +	return compare_mixer->compare(*(const snd_mixer_elem_t * const *) a,
> +				      *(const snd_mixer_elem_t * const *) b);
> +}

Because compare_mixer in this implementation is shared between all
threads, locking is required to make sure that different threads do not
attemt to use this variable at the same time.  My code does not use static
variables and therefore does not need such locking.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Re: alsa-lib patches

[Takashi Iwai]

At Sun, 15 May 2005 22:24:03 +0200,
Kevin F.Quinn <co@kevquinn.com> wrote:
> 
> 
> You also need to make it thread-safe, as Clements mentioned; the PaX
> team's proposal at https://bugs.gentoo.org/attachment.cgi?id=58918
> does that as well: 

The patch looks fine.  I applied it to CVS.

Thanks.

Takashi


-------------------------------------------------------
This SF.Net email is sponsored by Oracle Space Sweepstakes
Want to be the first software developer in space?
Enter now for the Oracle Space Sweepstakes!
http://ads.osdn.com/?ad_id=7412&alloc_id=16344&op=click