How much validation should spa_pod_is_array() and spa_pod_is_choice() do?
How much validation should spa_pod_is_array()
and spa_pod_is_choice()
do?
At a minimum, I think they should validate that:
- If the element size of an array is zero, the container size must also be zero.
- If the element size is nonzero, the container size must be a multiple of it, with no remainder. Otherwise the POD iterators will skip the last part.
- A choice must have enough elements for the type of choice it is:
- 1 for
SPA_CHOICE_None
,SPA_CHOICE_Enum
, andSPA_CHOICE_Flags
. - 3 for
SPA_CHOICE_Range
. - 4 for
SPA_CHOICE_Step
.
- 1 for
Should there be additional validation? For “simple” element types (None, Int, Long, Float, Double, Rectangle, Fraction), it’s safe to call the corresponding spa_pod_is_*()
functions on the child POD even if the container is empty, but this is not the case for String, Choice, Pod, Array, Object, Sequence, and possibly some others too. Furthermore, the POD iterators for arrays and choices assume that the elements are all the same size.
Therefore, I think that spa_pod_is_array()
and spa_pod_is_choice()
should check that either the type is a “simple” one, or that the size of the child POD is equal to the entire size of its parent (and that, therefore, the number of elements is 1). This ensures that one can always call spa_pod_is_*()
on the result of SPA_POD_ARRAY_CHILD()
/SPA_POD_CHOICE_CHILD()
and it will guarantee that every element obtained by iteration is valid. Furthermore, I think that only “simple” types should be allowed for Choice elements, unless the Choice type is SPA_CHOICE_None
. I’d prefer to disallow even that, but WirePlumber’s test suite creates a string Choice. IMO this is a bug in WirePlumber. It also creates a too-small Step
but that is a bug in the WirePlumber test suite.
Additionally, most types simply make no sense for non-SPA_CHOICE_None
Choice PODs. Only SPA_TYPE_Int
and SPA_TYPE_Long
make sense for SPA_CHOICE_Flags
, and only Int, Long, Float, Double, Rectangle, and Fraction make sense for Enum, Range, or Step Choices. The only exception is that a Bool Enum seems to be allowed.
For the fixed-size types, should spa_pod_is_array()
and spa_pod_is_choice()
also check that the element size is correct and (for Double and Long) that the pointer is properly aligned? Right now, spa_pod_is_*()
allow for lengths that are overly long and are not aligned, so without these checks in spa_pod_is_array()
and spa_pod_is_choice()
one could dereference misaligned pointers during iteration. Alternatively, the spa_pod_is_*()
functions could be changed to disallow overlong PODs and to perform an alignment check for Double and Long PODs. Doing both is the safest option, IMO.