[PATCH] ASoC: intel: avs: refactor strncpy usage in topology
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
A suitable replacement is `strscpy` [2].
There are some hopes that someday the `strncpy` api could be ripped out due to the vast number of suitable replacements (strscpy, strscpy_pad, strtomem, strtomem_pad, strlcpy) [1].
[1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
---
Link: https://github.com/KSPP/linux/issues/90 Signed-off-by: Justin Stitt justinstitt@google.com --- sound/soc/intel/avs/topology.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c index cdb4ec500261..45d0eb2a8e71 100644 --- a/sound/soc/intel/avs/topology.c +++ b/sound/soc/intel/avs/topology.c @@ -1388,12 +1388,12 @@ static int avs_route_load(struct snd_soc_component *comp, int index, port = __ffs(mach->mach_params.i2s_link_mask);
snprintf(buf, len, route->source, port); - strncpy((char *)route->source, buf, len); + strscpy((char *)route->source, buf, len); snprintf(buf, len, route->sink, port); - strncpy((char *)route->sink, buf, len); + strscpy((char *)route->sink, buf, len); if (route->control) { snprintf(buf, len, route->control, port); - strncpy((char *)route->control, buf, len); + strscpy((char *)route->control, buf, len); } }
--- base-commit: 0b4a9fdc9317440a71d4d4c264a5650bf4a90f3c change-id: 20230725-sound-soc-intel-avs-remove-deprecated-strncpy-2bc41a5a5f81
Best regards,
On Tue, Jul 25, 2023 at 10:08:38PM +0000, justinstitt@google.com wrote:
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
A suitable replacement is `strscpy` [2].
For future patches like this, can you include the rationale for _why_ strscpy() is suitable? (i.e. how did you check that it doesn't need zero-padding, the dest is expected to always be NUL-terminated, etc.)
I had fun looking through this code -- it's a bit of a maze! I can see in the initializer for "route" (soc_tplg_dapm_graph_elems_load()), that all the strings pointed at from "elem" are being checked for NUL-termination.
That this code is doing an in-place rewrite of the string is pretty unusual (i.e. specifically casting around the "const"), but it looks quite intentional. :)
There are some hopes that someday the `strncpy` api could be ripped out
This can be rephrased, perhaps, as:
There is a goal to eliminate the `strncpy` API in the kernel, as its use is too ambiguous, instead moving to the unambiguous replacements (strscpy, strscpy_pad, strtomem, strtomem_pad, strlcpy) [1].
due to the vast number of suitable replacements (strscpy, strscpy_pad, strtomem, strtomem_pad, strlcpy) [1].
^^^^ this triple-dash is going to cause some tooling to lose your S-o-b, as it indicates the end of the commit log.
Link: https://github.com/KSPP/linux/issues/90 Signed-off-by: Justin Stitt justinstitt@google.com
But otherwise, looks good:
Reviewed-by: Kees Cook keescook@chromium.org
-Kees
sound/soc/intel/avs/topology.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c index cdb4ec500261..45d0eb2a8e71 100644 --- a/sound/soc/intel/avs/topology.c +++ b/sound/soc/intel/avs/topology.c @@ -1388,12 +1388,12 @@ static int avs_route_load(struct snd_soc_component *comp, int index, port = __ffs(mach->mach_params.i2s_link_mask);
snprintf(buf, len, route->source, port);
strncpy((char *)route->source, buf, len);
strscpy((char *)route->source, buf, len);
strncpy((char *)route->sink, buf, len);
strscpy((char *)route->sink, buf, len);
strncpy((char *)route->control, buf, len);
strscpy((char *)route->control, buf, len);
base-commit: 0b4a9fdc9317440a71d4d4c264a5650bf4a90f3c change-id: 20230725-sound-soc-intel-avs-remove-deprecated-strncpy-2bc41a5a5f81
Best regards,
Justin Stitt justinstitt@google.com
On 7/26/2023 12:08 AM, justinstitt@google.com wrote:
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
A suitable replacement is `strscpy` [2].
There are some hopes that someday the `strncpy` api could be ripped out due to the vast number of suitable replacements (strscpy, strscpy_pad, strtomem, strtomem_pad, strlcpy) [1].
Link: https://github.com/KSPP/linux/issues/90 Signed-off-by: Justin Stitt justinstitt@google.com
sound/soc/intel/avs/topology.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c index cdb4ec500261..45d0eb2a8e71 100644 --- a/sound/soc/intel/avs/topology.c +++ b/sound/soc/intel/avs/topology.c @@ -1388,12 +1388,12 @@ static int avs_route_load(struct snd_soc_component *comp, int index, port = __ffs(mach->mach_params.i2s_link_mask);
snprintf(buf, len, route->source, port);
strncpy((char *)route->source, buf, len);
strscpy((char *)route->source, buf, len);
strncpy((char *)route->sink, buf, len);
strscpy((char *)route->sink, buf, len);
strncpy((char *)route->control, buf, len);
strscpy((char *)route->control, buf, len);
In this case snprintf adds NUL at the end and we strncpy using same size, so there should always be NUL at the end, so replacing it with strscpy shouldn't really change anything, so
Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
On Tue, Jul 25, 2023 at 10:08:38PM +0000, justinstitt@google.com wrote:
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
A suitable replacement is `strscpy` [2].
There are some hopes that someday the `strncpy` api could be ripped out due to the vast number of suitable replacements (strscpy, strscpy_pad, strtomem, strtomem_pad, strlcpy) [1].
Link: https://github.com/KSPP/linux/issues/90 Signed-off-by: Justin Stitt justinstitt@google.com
You've put your signoff after a --- which means it gets deleted when applied, don't do this. The Signoff should be start of the main changelog.
On Tue, 25 Jul 2023 22:08:38 +0000, justinstitt@google.com wrote:
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
A suitable replacement is `strscpy` [2].
There are some hopes that someday the `strncpy` api could be ripped out due to the vast number of suitable replacements (strscpy, strscpy_pad, strtomem, strtomem_pad, strlcpy) [1].
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: intel: avs: refactor strncpy usage in topology commit: f6500ec12c1ec745fbec20fd4734b832bbfd4aac
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (4)
-
Amadeusz Sławiński -
justinstitt＠google.com -
Kees Cook -
Mark Brown