[alsa-lib][PATCH 0/6] add API of equality and comparison for a pair of control element IDs

[Jaroslav Kysela]

Dne 18. 03. 21 v 17:37 Takashi Sakamoto napsal(a):
> On Thu, Mar 18, 2021 at 12:42:30PM +0100, Jaroslav Kysela wrote:
>> Dne 18. 03. 21 v 11:30 Takashi Sakamoto napsal(a):
>>> Hi,
>>>
>>> This patchset is a fix for bug issued in the message thread[1].
>>>
>>> In this development period, alsa-lib got new API as implementation for
>>> one of comparison algorithms to a pair of control element IDs. However,
>>> it has several issues.
>>>
>>> At first, the name, 'snd_ctl_elem_id_compare()', is inappropriate since it
>>> implements one of comparison algorithms. The name itself implies the
>>> algorithm is single and unique for control element ID. However, the
>>> target structure, 'struct snd_ctl_elem_id', is hybrid and compound one.
>>> We can not find such single and unique comparison algorithm for it.
>>>
>>> Secondary, it subtracts a pair of values in fields of 'unsigned int' type
>>> in storage size of the type. It brings integer overflow.
>>
>> I don't think that this extra handling is really required. The unsigned /
>> signed conversions are well known and the overflow results with a negative
>> signed value. Why add more branches to the instruction chain?
>  
> For this kind of question, it's preferable to write actual test:
> 
> ```
> int main()
> {
>   snd_ctl_elem_id_t *l, *r;
> 
>   snd_ctl_elem_id_alloca(&l);
>   snd_ctl_elem_id_alloca(&r);
> 
>   snd_ctl_elem_id_set_device(l, 0);
>   snd_ctl_elem_id_set_device(r, UINT_MAX);
> 
>   assert(snd_ctl_elem_id_compare(l, r) < 0);
> 
>   return 0;
> }
> ```
> 
> The assertion hits. For conversion detail:
> 
> ```
> $ cat test1.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <limits.h>
> 
> int main()
> {
> 	unsigned int a, b;
> 	int diff;
> 
> 	a = 0;
> 	b = 10;
> 	diff = a - b;
> 	printf("%08x\n", diff);
> 
> 	a = 0;
> 	b = UINT_MAX;
> 	diff = a - b;
> 	printf("%08x\n", diff);
> 
> 	return EXIT_SUCCESS;
> }
> ```
> 
> The above test results in 0x00000001 for -UINT_MAX case under x386/x86_64,
> like:
> 
> ```
> $ gcc -m32 -o ./test1 ./test1.c ; ./test1
> fffffff6
> 00000001
> $ gcc -m64 -o ./test1 ./test1.c ; ./test1
> fffffff6
> 00000001
> ```
> 
> We can see integer overflow in both machine architectures due to
> calculation under 32 bit storage.
> 
> Well, let us prepare 64 bit storage for both of minuend and subtrahend
> to get negative value in 64 bit storage. In the case, narrower conversion
> to 32 bit integer is unavoidable since it's assigned to integer value
> returned from the function in your implementation. In the case, converted
> value is _not_ negative according to assignment rule in C language
> specification.
> 
> ```
> $ cat test2.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <limits.h>
> 
> int main()
> {
> 	unsigned int a, b;
> 	long long diff;
> 	int ret;
> 
> 	a = 0;
> 	b = UINT_MAX;
> 
> 	// Calculate under 64 bit storage, then assign to 64 bit storage.
> 	diff = (long long)a - (long long)b;
> 	printf("%016llx\n", diff);
> 
> 	// Narrower conversion occurs now.
> 	ret = (int)diff;
> 	printf("%08x\n", ret);
> 
> 	return EXIT_SUCCESS;
> }
> $ gcc -m32 -o ./test2 ./test2.c ; ./test2
> ffffffff00000001
> 00000001
> $ gcc -m64 -o ./test2 ./test2.c ; ./test2
> ffffffff00000001
> 00000001
> ```
> 
> We can see easy example in the clause of 'Assignment operators' in the
> specification. This is the reason to use condition branches in the patchset.

The point is that none of the compared unsigned fields is really above the
31-bit range, so you're trying to resolve an academical problem instead to add
the debug checks (asserts) if the input values are in the acceptable range.
Only the numid functions require this.

					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.