On Tue, Feb 11, 2025 at 03:10:38PM +0000, Richard Fitzgerald wrote:
On 11/02/2025 3:00 pm, Thomas Weißschuh wrote:
The char pointers in 'struct cs_dsp_mock_coeff_def' are expected to point to C strings. They need to be terminated by a null byte. However the code does not allocate that trailing null byte and only works if by chance the allocation is followed by such a null byte.
Refactor the repeated string allocation logic into a new helper which makes sure the terminating null is always present. It also makes the code more readable.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de Fixes: 83baecd92e7c ("firmware: cs_dsp: Add KUnit testing of control parsing") Cc: stable@vger.kernel.org
.../cirrus/test/cs_dsp_test_control_parse.c | 51 ++++++++-------------- 1 file changed, 19 insertions(+), 32 deletions(-)
diff --git a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c index cb90964740ea351113dac274f0366de7cedfd3d1..942ba1af5e7c1e47e8a2fbe548a7993b94f96515 100644 --- a/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c +++ b/drivers/firmware/cirrus/test/cs_dsp_test_control_parse.c @@ -73,6 +73,18 @@ static const struct cs_dsp_mock_coeff_def mock_coeff_template = { .length_bytes = 4, }; +static char *cs_dsp_ctl_alloc_test_string(struct kunit *test, char c, size_t len) +{
- char *str;
- str = kunit_kmalloc(test, len + 1, GFP_KERNEL);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
- memset(str, c, len);
- str[len] = '\0';
- return str;
+}
- /* Algorithm info block without controls should load */ static void cs_dsp_ctl_parse_no_coeffs(struct kunit *test) {
@@ -160,12 +172,8 @@ static void cs_dsp_ctl_parse_max_v1_name(struct kunit *test) struct cs_dsp_mock_coeff_def def = mock_coeff_template; struct cs_dsp_coeff_ctl *ctl; struct firmware *wmfw;
- char *name;
- name = kunit_kzalloc(test, 256, GFP_KERNEL);
This allocates 256 bytes of zero-filled memory...
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, name);
- memset(name, 'A', 255);
... and this fills the first 255 bytes, leaving the last byte still as zero. So the string is zero-terminated. I don't see a problem here.
This single instance it is indeed correct. In all other five it's broken.
Just fix the other allocs to be kzalloc with the correct length?
If you prefer that, sure I can change it.
Personally I like the helper much better. One does not have to look at a dense block of code to see what the actual intention is. Assuming the location in cs_dsp_ctl_parse_max_v1_name() was fixed when some breakage was observed, with a helper it would have been fixed for all locations and not crept into upstream code.
Thomas