[PATCH v2] ALSA: wavefront: use scnprintf for longname construction

[PATCH v2] ALSA: wavefront: use scnprintf for longname construction

[moonafterrain]

From: Junrui Luo <moonafterrain@outlook.com>

Replace sprintf() calls with scnprintf() and a new scnprintf_append()
helper function when constructing card->longname. This improves code
readability and provides bounds checking for the 80-byte buffer.

While the current parameter ranges don't cause overflow in practice,
using safer string functions follows kernel best practices and makes
the code more maintainable.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
Changes in v2:
- Replace sprintf() calls with scnprintf() and a new scnprintf_append()
- Link to v1: https://lore.kernel.org/all/ME2PR01MB3156CEC4F31F253C9B540FB7AFFDA@ME2PR01MB3156.ausprd01.prod.outlook.com/
---
 sound/isa/wavefront/wavefront.c | 50 +++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/sound/isa/wavefront/wavefront.c b/sound/isa/wavefront/wavefront.c
index 07c68568091d..047dd54f77d4 100644
--- a/sound/isa/wavefront/wavefront.c
+++ b/sound/isa/wavefront/wavefront.c
@@ -333,6 +333,19 @@ static int snd_wavefront_card_new(struct device *pdev, int dev,
 	return 0;
 }
 
+__printf(3, 4) static int scnprintf_append(char *buf, size_t size, const char *fmt, ...)
+{
+	va_list args;
+	size_t len = strlen(buf);
+
+	if (len >= size)
+		return len;
+	va_start(args, fmt);
+	len = vscnprintf(buf + len, size - len, fmt, args);
+	va_end(args);
+	return len;
+}
+
 static int
 snd_wavefront_probe (struct snd_card *card, int dev)
 {
@@ -492,26 +505,27 @@ snd_wavefront_probe (struct snd_card *card, int dev)
 	   length restrictions
 	*/
 
-	sprintf(card->longname, "%s PCM 0x%lx irq %d dma %d",
-		card->driver,
-		chip->port,
-		cs4232_pcm_irq[dev],
-		dma1[dev]);
+	scnprintf(card->longname, sizeof(card->longname),
+		  "%s PCM 0x%lx irq %d dma %d",
+		  card->driver,
+		  chip->port,
+		  cs4232_pcm_irq[dev],
+		  dma1[dev]);
 
 	if (dma2[dev] >= 0 && dma2[dev] < 8)
-		sprintf(card->longname + strlen(card->longname), "&%d", dma2[dev]);
-
-	if (cs4232_mpu_port[dev] > 0 && cs4232_mpu_port[dev] != SNDRV_AUTO_PORT) {
-		sprintf (card->longname + strlen (card->longname), 
-			 " MPU-401 0x%lx irq %d",
-			 cs4232_mpu_port[dev],
-			 cs4232_mpu_irq[dev]);
-	}
-
-	sprintf (card->longname + strlen (card->longname), 
-		 " SYNTH 0x%lx irq %d",
-		 ics2115_port[dev],
-		 ics2115_irq[dev]);
+		scnprintf_append(card->longname, sizeof(card->longname),
+				 "&%d", dma2[dev]);
+
+	if (cs4232_mpu_port[dev] > 0 && cs4232_mpu_port[dev] != SNDRV_AUTO_PORT)
+		scnprintf_append(card->longname, sizeof(card->longname),
+				 " MPU-401 0x%lx irq %d",
+				 cs4232_mpu_port[dev],
+				 cs4232_mpu_irq[dev]);
+
+	scnprintf_append(card->longname, sizeof(card->longname),
+			 " SYNTH 0x%lx irq %d",
+			 ics2115_port[dev],
+			 ics2115_irq[dev]);
 
 	return snd_card_register(card);
 }	
-- 
2.51.1.dirty

Re: [PATCH v2] ALSA: wavefront: use scnprintf for longname construction

[Takashi Iwai]

On Sun, 02 Nov 2025 16:32:39 +0100,
moonafterrain@outlook.com wrote:
> 
> From: Junrui Luo <moonafterrain@outlook.com>
> 
> Replace sprintf() calls with scnprintf() and a new scnprintf_append()
> helper function when constructing card->longname. This improves code
> readability and provides bounds checking for the 80-byte buffer.
> 
> While the current parameter ranges don't cause overflow in practice,
> using safer string functions follows kernel best practices and makes
> the code more maintainable.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> ---
> Changes in v2:
> - Replace sprintf() calls with scnprintf() and a new scnprintf_append()
> - Link to v1: https://lore.kernel.org/all/ME2PR01MB3156CEC4F31F253C9B540FB7AFFDA@ME2PR01MB3156.ausprd01.prod.outlook.com/

Well, my suggestion was that we can apply such conversions once if a
*generic* helper becomes available; that is, propose
scnprintf_append() to be put in include/linux/string.h or whatever (I
guess better in *.c instead of inline), and once if it's accepted, we
can convert the relevant places (there are many, not only
wavefront.c).

BTW:

> +__printf(3, 4) static int scnprintf_append(char *buf, size_t size, const char *fmt, ...)
> +{
> +	va_list args;
> +	size_t len = strlen(buf);
> +
> +	if (len >= size)
> +		return len;
> +	va_start(args, fmt);
> +	len = vscnprintf(buf + len, size - len, fmt, args);
> +	va_end(args);
> +	return len;

The above should be
	len += vscnprintf(buf + len, size - len, fmt, args);
so that it returns the full size of the string.
If it were in user-space, I'd check a negative error code, but the
Linux kernel implementation doesn't return a negative error code, so
far.
I see it's a copy from a code snipped I suggested which already
contained the error :)

Also, it might be safer to use strnlen() instead of strlen() for
avoiding a potential out-of-bound access.


thanks,

Takashi

Re: [PATCH v2] ALSA: wavefront: use scnprintf for longname construction

[222 Summ]

Hi Takashi,

Thank you for your detailed feedback on the v2 patch. I've prepared a v3
patch series that incorporates your suggestions.

Based on your comments, I've made the following changes:

1. Split the patch into two parts:
   - Patch 1/2: Adds `scnprintf_append()` to `lib/vsprintf.c`
   - Patch 2/2: Converts `wavefront.c` to use it
2. Fixed the return value you pointed out
3. Used strnlen() instead of strlen() for safety

I plan to submit this as a two-patch series. However, before I send it, I'd like to confirm a few things:

1. Is this approach (adding the helper to lib/vsprintf.c) what you had in
   mind? Or would you prefer a different location?
2. Would you recommend sending both patches together, or waiting until
patch 1/2 is reviewed and accepted before submitting patch 2/2?

The implementation of scnprintf_append() is shown below:
+int scnprintf_append(char *buf, size_t size, const char *fmt, ...)
+{
+	va_list args;
+	size_t len;
+
+	len = strnlen(buf, size);
+	if (len >= size)
+		return len;
+	va_start(args, fmt);
+	len += vscnprintf(buf + len, size - len, fmt, args);
+	va_end(args);
+	return len;
+}

Thanks again for your guidance.

Best regards,
Junrui

________________________________________
From: Takashi Iwai <tiwai@suse.de>
Sent: Tuesday, November 4, 2025 18:01
To: moonafterrain@outlook.com <moonafterrain@outlook.com>
Cc: Jaroslav Kysela <perex@perex.cz>; Takashi Iwai <tiwai@suse.com>; linux-sound@vger.kernel.org <linux-sound@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; stable@vger.kernel.org <stable@vger.kernel.org>; Yuhao Jiang <danisjiang@gmail.com>
Subject: Re: [PATCH v2] ALSA: wavefront: use scnprintf for longname construction
 
On Sun, 02 Nov 2025 16:32:39 +0100,
moonafterrain@outlook.com wrote:
>
> From: Junrui Luo <moonafterrain@outlook.com>
>
> Replace sprintf() calls with scnprintf() and a new scnprintf_append()
> helper function when constructing card->longname. This improves code
> readability and provides bounds checking for the 80-byte buffer.
>
> While the current parameter ranges don't cause overflow in practice,
> using safer string functions follows kernel best practices and makes
> the code more maintainable.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> ---
> Changes in v2:
> - Replace sprintf() calls with scnprintf() and a new scnprintf_append()
> - Link to v1: https://lore.kernel.org/all/ME2PR01MB3156CEC4F31F253C9B540FB7AFFDA@ME2PR01MB3156.ausprd01.prod.outlook.com/

Well, my suggestion was that we can apply such conversions once if a
*generic* helper becomes available; that is, propose
scnprintf_append() to be put in include/linux/string.h or whatever (I
guess better in *.c instead of inline), and once if it's accepted, we
can convert the relevant places (there are many, not only
wavefront.c).

BTW:

> +__printf(3, 4) static int scnprintf_append(char *buf, size_t size, const char *fmt, ...)
> +{
> +     va_list args;
> +     size_t len = strlen(buf);
> +
> +     if (len >= size)
> +             return len;
> +     va_start(args, fmt);
> +     len = vscnprintf(buf + len, size - len, fmt, args);
> +     va_end(args);
> +     return len;

The above should be
        len += vscnprintf(buf + len, size - len, fmt, args);
so that it returns the full size of the string.
If it were in user-space, I'd check a negative error code, but the
Linux kernel implementation doesn't return a negative error code, so
far.
I see it's a copy from a code snipped I suggested which already
contained the error :)

Also, it might be safer to use strnlen() instead of strlen() for
avoiding a potential out-of-bound access.


thanks,

Takashi

Re: [PATCH v2] ALSA: wavefront: use scnprintf for longname construction

[Takashi Iwai]

On Wed, 05 Nov 2025 05:28:27 +0100,
222 Summ wrote:
> 
> Hi Takashi,
> 
> Thank you for your detailed feedback on the v2 patch. I've prepared a v3
> patch series that incorporates your suggestions.
> 
> Based on your comments, I've made the following changes:
> 
> 1. Split the patch into two parts:
>    - Patch 1/2: Adds `scnprintf_append()` to `lib/vsprintf.c`
>    - Patch 2/2: Converts `wavefront.c` to use it
> 2. Fixed the return value you pointed out
> 3. Used strnlen() instead of strlen() for safety
> 
> I plan to submit this as a two-patch series. However, before I send it, I'd like to confirm a few things:
> 
> 1. Is this approach (adding the helper to lib/vsprintf.c) what you had in
>    mind? Or would you prefer a different location?

IMO lib/vsprintf.c should be fine.

> 2. Would you recommend sending both patches together, or waiting until
> patch 1/2 is reviewed and accepted before submitting patch 2/2?

You can try patching not only wavefront.c but covering a few others in
the series at first for showing that it really makes sense to be a
generic helper function.  And, submitting the whole might be better to
understand what's the use and effect, too.

Note that there can be suggestions for other forms, e.g. there have
been some progresses for the continuous string processing, IIRC.
So this is merely one example to achieve the goal.

The merit of this way would be its simplicity, though: you can replace
the code with a single function call without keeping anything else,
and the handling logic is quite straightforward.

> The implementation of scnprintf_append() is shown below:
> +int scnprintf_append(char *buf, size_t size, const char *fmt, ...)
> +{
> +	va_list args;
> +	size_t len;
> +
> +	len = strnlen(buf, size);
> +	if (len >= size)
> +		return len;
> +	va_start(args, fmt);
> +	len += vscnprintf(buf + len, size - len, fmt, args);
> +	va_end(args);
> +	return len;
> +}

This should be with EXPORT_SYMBOL_GPL() (or EXPORT_SYMBOL() is any
user is non-GPL).


thanks,

Takashi

> 
> Thanks again for your guidance.
> 
> Best regards,
> Junrui
> 
> ________________________________________
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Tuesday, November 4, 2025 18:01
> To: moonafterrain@outlook.com <moonafterrain@outlook.com>
> Cc: Jaroslav Kysela <perex@perex.cz>; Takashi Iwai <tiwai@suse.com>; linux-sound@vger.kernel.org <linux-sound@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; stable@vger.kernel.org <stable@vger.kernel.org>; Yuhao Jiang <danisjiang@gmail.com>
> Subject: Re: [PATCH v2] ALSA: wavefront: use scnprintf for longname construction
>  
> On Sun, 02 Nov 2025 16:32:39 +0100,
> moonafterrain@outlook.com wrote:
> >
> > From: Junrui Luo <moonafterrain@outlook.com>
> >
> > Replace sprintf() calls with scnprintf() and a new scnprintf_append()
> > helper function when constructing card->longname. This improves code
> > readability and provides bounds checking for the 80-byte buffer.
> >
> > While the current parameter ranges don't cause overflow in practice,
> > using safer string functions follows kernel best practices and makes
> > the code more maintainable.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> > ---
> > Changes in v2:
> > - Replace sprintf() calls with scnprintf() and a new scnprintf_append()
> > - Link to v1: https://lore.kernel.org/all/ME2PR01MB3156CEC4F31F253C9B540FB7AFFDA@ME2PR01MB3156.ausprd01.prod.outlook.com/
> 
> Well, my suggestion was that we can apply such conversions once if a
> *generic* helper becomes available; that is, propose
> scnprintf_append() to be put in include/linux/string.h or whatever (I
> guess better in *.c instead of inline), and once if it's accepted, we
> can convert the relevant places (there are many, not only
> wavefront.c).
> 
> BTW:
> 
> > +__printf(3, 4) static int scnprintf_append(char *buf, size_t size, const char *fmt, ...)
> > +{
> > +     va_list args;
> > +     size_t len = strlen(buf);
> > +
> > +     if (len >= size)
> > +             return len;
> > +     va_start(args, fmt);
> > +     len = vscnprintf(buf + len, size - len, fmt, args);
> > +     va_end(args);
> > +     return len;
> 
> The above should be
>         len += vscnprintf(buf + len, size - len, fmt, args);
> so that it returns the full size of the string.
> If it were in user-space, I'd check a negative error code, but the
> Linux kernel implementation doesn't return a negative error code, so
> far.
> I see it's a copy from a code snipped I suggested which already
> contained the error :)
> 
> Also, it might be safer to use strnlen() instead of strlen() for
> avoiding a potential out-of-bound access.
> 
> 
> thanks,
> 
> Takashi