[alsa-devel] [PATCH] ASoC: soc-dapm: Fix comparison of pointers
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; }
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.
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
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.
participants (2)
-
Mark Brown
-
Rasmus Villemoes