Re: [PATCH v2] ALSA: hda/generic: Replace strlcat() with strscpy()

[Takashi Iwai <tiwai@suse.de>]

On Wed, 29 Jul 2026 18:06:52 +0200,
Ian Bridges wrote:
> 
> In preparation for removing the strlcat() API[1], replace its uses in
> fill_pcm_stream_name() and get_jack_mode_name(). Both appends become
> bounded strscpy() calls at a known position.
> 
> In fill_pcm_stream_name() the trim walk itself lands on the append
> position. It stops at the trim point when the name has a decoration
> to drop, and at the terminating NUL otherwise, so the suffix
> overwrites from there and no length bookkeeping is needed.
> 
> get_jack_mode_name() measures the label once with strnlen() because
> snd_hda_get_pin_label() does not return a length.
> 
> The produced strings are unchanged.
> 
> Link: https://github.com/KSPP/linux/issues/370 [1]
> Signed-off-by: Ian Bridges <icb@fastmail.org>
> ---
> v2: land on the append position instead of measuring before every
> append. In fill_pcm_stream_name() the trim walk already finishes at
> the append position, so the suffix goes down with one bounded
> strscpy() and no length bookkeeping, matching the pattern of commit
> 86dc52c5c96e ("ALSA: usb-audio: simplify mixer control name
> handling"). get_jack_mode_name() keeps one strnlen() because
> snd_hda_get_pin_label() returns no length. Other shapes were built
> and measured for this respin, including the seq_buf form suggested
> in the thread. That one moves the fill-only-when-empty guard of
> fill_pcm_stream_name() into all three callers, because
> seq_buf_init() clears the first byte, and every call site grew.
> 
> v1: https://lore.kernel.org/all/alaPxW5S5PT_-kZi@dev/

Honestly speaking, I still don't like those changes, sorry.

My argument is about neither code correctness nor efficiency.
Instead, it's about that such a open code makes harder to understand
the intention of the code.

The meaning of strlcat() is clear: append a suffix string in a safe
way.  Meanwhile, when you open-code it:

	size_t used;
	....
 	snd_hda_get_pin_label(codec, pin, &spec->autocfg, name, name_len, &idx);
	used = strnlen(name, name_len);
	strscpy(name + used, " Jack Mode", name_len - used);

it's no longer obvious at a first glance, and you'd need to think of
it much longer -- what does this and whether it's really safe.

So, if we must inevitably drop strlcat() function, it's better to
rearrange the caller side beforehand, e.g. replace strlcat() call to
a local function like append_suffix(), in order to make the meaning
of the code clearer.  Then we can replace the code in append_suffix().

// append a suffix string safely; equivalent with strlcat()
static void append_suffix(char *str, const char *suffix, size_t size)
{
	size_t used = strnlen(str, size);
	strscpy(str + used, suffix, size - used);
}

....
	snd_hda_get_pin_label(codec, pin, &spec->autocfg, name, name_len, &idx);
	append_suffix(name, " Jack Mode", name_len);

Since I've been already working on a cleanup of the relevant code, I'm
going to change strlcat() to append_suffix() (but with strlcat()) as a
preparation, too.  Once when we decide to drop strlcat() actually, we
can change the code in that function.


thanks,

Takashi