[PATCH] ASoC: soc-dapm: Fix comparison of pointers

[PATCH] ASoC: soc-dapm: Fix comparison of pointers

[Rasmus Villemoes]

The idiom 'return a - b;' often used in comparison functions is wrong
unless one is certain the values being compared lie in a sufficiently
small range. In this case dapm_seq_compare would also return 0 if the
->dapm pointers happened to differ by a multiple of 2^32.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---

Notes:
    I don't know if the other occurences of the idiom in this function are
    ok; that depends on whether the values always lie in [0, 2^31-1].

 sound/soc/soc-dapm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index c61cb9cedbcd..a894a7c71557 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1255,8 +1255,10 @@ static int dapm_seq_compare(struct snd_soc_dapm_widget *a,
 	}
 	if (a->reg != b->reg)
 		return a->reg - b->reg;
-	if (a->dapm != b->dapm)
-		return (unsigned long)a->dapm - (unsigned long)b->dapm;
+	if (a->dapm < b->dapm)
+		return -1;
+	if (a->dapm > b->dapm)
+		return 1;
 
 	return 0;
 }
-- 
2.1.3

Re: [PATCH] ASoC: soc-dapm: Fix comparison of pointers

[Mark Brown]

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

On Tue, Dec 09, 2014 at 10:53:41PM +0100, Rasmus Villemoes wrote:

>  	if (a->reg != b->reg)
>  		return a->reg - b->reg;
> -	if (a->dapm != b->dapm)
> -		return (unsigned long)a->dapm - (unsigned long)b->dapm;
> +	if (a->dapm < b->dapm)
> +		return -1;
> +	if (a->dapm > b->dapm)
> +		return 1;

If we're worrying about standards conformance type stuff here this is
also buggy since it's out of spec to compare pointers that are not part
of the same array like this.  Casting to uintptr_t is a better fix here.

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

Re: [PATCH] ASoC: soc-dapm: Fix comparison of pointers

[Rasmus Villemoes]

On Fri, Dec 12 2014, Mark Brown <broonie@kernel.org> wrote:

> On Tue, Dec 09, 2014 at 10:53:41PM +0100, Rasmus Villemoes wrote:
>
>>  	if (a->reg != b->reg)
>>  		return a->reg - b->reg;
>> -	if (a->dapm != b->dapm)
>> -		return (unsigned long)a->dapm - (unsigned long)b->dapm;
>> +	if (a->dapm < b->dapm)
>> +		return -1;
>> +	if (a->dapm > b->dapm)
>> +		return 1;
>
> If we're worrying about standards conformance type stuff here this is
> also buggy since it's out of spec to compare pointers that are not part
> of the same array like this.  Casting to uintptr_t is a better fix here.

I'm not worrying about standards conformance. Casting pointers to and
from [unsigned] long is done everywhere in the kernel, and that's
fine. But subtracting ints or longs and using the sign of the result for
comparison is simply wrong. See acbbe6fbb2 (kcmp: fix standard
comparison bug) for the long story. In this case the narrowing (when
BITS_PER_LONG==64) of the result from long to int just makes it even
more wrong.

Rasmus

Re: [PATCH] ASoC: soc-dapm: Fix comparison of pointers

[Mark Brown]

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

On Fri, Dec 12, 2014 at 02:43:27PM +0100, Rasmus Villemoes wrote:
> On Fri, Dec 12 2014, Mark Brown <broonie@kernel.org> wrote:

> > If we're worrying about standards conformance type stuff here this is
> > also buggy since it's out of spec to compare pointers that are not part
> > of the same array like this.  Casting to uintptr_t is a better fix here.

> I'm not worrying about standards conformance. Casting pointers to and
> from [unsigned] long is done everywhere in the kernel, and that's
> fine. But subtracting ints or longs and using the sign of the result for
> comparison is simply wrong. See acbbe6fbb2 (kcmp: fix standard
> comparison bug) for the long story. In this case the narrowing (when
> BITS_PER_LONG==64) of the result from long to int just makes it even
> more wrong.

*sigh*  If that's what you meant you should've said so in your commit
log since that's distinctly non-obvious.  In any case, regardless of why
you originally wanted to do this the fact remains that your new version
is out of spec.

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