Undefined Behaviour in volk_8u_x4_conv_k7_r2_8u

[argilo]

UBSAN reports undefined behaviour in this code:

volk/kernels/volk/volk_8u_x4_conv_k7_r2_8u.h

Lines 112 to 113 in 42f57cd

	d->w[i / (sizeof(unsigned int) * 8 / 2) +
	s * (sizeof(decision_t) / sizeof(unsigned int))] |=

Here w is an unsigned int[2], and the s * (sizeof(decision_t) / sizeof(unsigned int)) calculation intentionally reaches outside the bounds of the array to access the sth element of the outer decision_t array d.

It looks like this hack became necessary because sizeof(decision_t) is 8, and yet it is marked as being 16-byte aligned; because of this, accessing d[1] (or any other odd offset) fails due to misalignment. I suspect that the 16-byte alignment of decision_t is incorrect or unnecessary.

[jdemel]

Thanks for finding this issue. Bugs in the FEC decoder code are a nightmare because or test infrastructure is unsuitable for this kernel.

Not directly related to the bug at hand, but:
I had the idea to introduce gtest to VOLK. Writing more extensive tests for this kernel with specific input might be worthwhile.

[argilo]

It looks like it will not be possible to avoid Undefined Behaviour in this kernel without changing the API. Decisions are returned as an array of unsigned char and yet they're actually packed as unsigned int. VOLK and GNU Radio both use a decision_t union to switch between the two in an unsafe way.

The decisions are packed bits. I think storing them in unsigned char would make more sense. Even though the function signature would remain the same, this would be an API change because the bytes would be in order, while currently each group of four bytes is reversed (on little-endian platforms, anyway). So we'd have to deprecate this kernel and create a new one.