[PATCH] ALSA: Warn when control names are truncated

[PATCH] ALSA: Warn when control names are truncated

[Mark Brown]

This is likely to confuse user interfaces since the end of the control
name is interpreted (eg, "Volume", "Switch").

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/core/control.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 6d71f9a..1257ca8 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -225,8 +225,12 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol,
 	kctl.id.iface = ncontrol->iface;
 	kctl.id.device = ncontrol->device;
 	kctl.id.subdevice = ncontrol->subdevice;
-	if (ncontrol->name)
+	if (ncontrol->name) {
+		if (strlen(ncontrol->name) > sizeof(kctl.id.name))
+			snd_printk(KERN_WARNING "Control name '%s' truncated\n",
+				   ncontrol->name);
 		strlcpy(kctl.id.name, ncontrol->name, sizeof(kctl.id.name));
+	}
 	kctl.id.index = ncontrol->index;
 	kctl.count = ncontrol->count ? ncontrol->count : 1;
 	access = ncontrol->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
-- 
1.5.6.5

Re: [PATCH] ALSA: Warn when control names are truncated

[Takashi Iwai]

At Wed, 29 Oct 2008 14:14:54 +0000,
Mark Brown wrote:
> 
> -	if (ncontrol->name)
> +	if (ncontrol->name) {
> +		if (strlen(ncontrol->name) > sizeof(kctl.id.name))

This should be '>=' since strlen() doesn't count the terminator.


thanks,

Takashi

Re: [PATCH] ALSA: Warn when control names are truncated

[Mark Brown]

On Wed, Oct 29, 2008 at 03:28:50PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > -	if (ncontrol->name)
> > +	if (ncontrol->name) {
> > +		if (strlen(ncontrol->name) > sizeof(kctl.id.name))

> This should be '>=' since strlen() doesn't count the terminator.

Oh, sorry - I'd misread strlcpy() as strncpy() (which would mean that
the termination character wasn't needed).

[PATCH] ALSA: Warn when control names are truncated

[Mark Brown]

This is likely to confuse user interfaces since the end of the control
name is interpreted (eg, "Volume", "Switch").

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/core/control.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 6d71f9a..b0bf426 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -225,8 +225,13 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol,
 	kctl.id.iface = ncontrol->iface;
 	kctl.id.device = ncontrol->device;
 	kctl.id.subdevice = ncontrol->subdevice;
-	if (ncontrol->name)
+	if (ncontrol->name) {
 		strlcpy(kctl.id.name, ncontrol->name, sizeof(kctl.id.name));
+		if (strcmp(ncontrol->name, kctl.id.name) != 0)
+			snd_printk(KERN_WARNING
+				   "Control name '%s' truncated to '%s'\n",
+				   ncontrol->name, kctl.id.name);
+	}
 	kctl.id.index = ncontrol->index;
 	kctl.count = ncontrol->count ? ncontrol->count : 1;
 	access = ncontrol->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
-- 
1.5.6.5

Re: [PATCH] ALSA: Warn when control names are truncated

[Takashi Iwai]

At Wed, 29 Oct 2008 14:40:30 +0000,
Mark Brown wrote:
> 
> This is likely to confuse user interfaces since the end of the control
> name is interpreted (eg, "Volume", "Switch").
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Thanks, applied now.


Takashi

> ---
>  sound/core/control.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 6d71f9a..b0bf426 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -225,8 +225,13 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol,
>  	kctl.id.iface = ncontrol->iface;
>  	kctl.id.device = ncontrol->device;
>  	kctl.id.subdevice = ncontrol->subdevice;
> -	if (ncontrol->name)
> +	if (ncontrol->name) {
>  		strlcpy(kctl.id.name, ncontrol->name, sizeof(kctl.id.name));
> +		if (strcmp(ncontrol->name, kctl.id.name) != 0)
> +			snd_printk(KERN_WARNING
> +				   "Control name '%s' truncated to '%s'\n",
> +				   ncontrol->name, kctl.id.name);
> +	}
>  	kctl.id.index = ncontrol->index;
>  	kctl.count = ncontrol->count ? ncontrol->count : 1;
>  	access = ncontrol->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
> -- 
> 1.5.6.5
> 

Re: [PATCH] ALSA: Warn when control names are truncated

[Jaroslav Kysela]

On Wed, 29 Oct 2008, Takashi Iwai wrote:

> At Wed, 29 Oct 2008 14:40:30 +0000,
> Mark Brown wrote:
> > 
> > This is likely to confuse user interfaces since the end of the control
> > name is interpreted (eg, "Volume", "Switch").
> > 
> > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> Thanks, applied now.
> 
> 
> > +		if (strcmp(ncontrol->name, kctl.id.name) != 0)

Maybe better is to just use end char comparsion to save few CPU ticks:

if (kctl.id.name[sizeof(kctl.id.name)-2] != '\0' &&
    ncontrol->name[sizeof(kctl.id.name)-1] != '\0')
	print_warning_here;

					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH] ALSA: Warn when control names are truncated

[Takashi Iwai]

At Wed, 29 Oct 2008 15:51:40 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Wed, 29 Oct 2008, Takashi Iwai wrote:
> 
> > At Wed, 29 Oct 2008 14:40:30 +0000,
> > Mark Brown wrote:
> > > 
> > > This is likely to confuse user interfaces since the end of the control
> > > name is interpreted (eg, "Volume", "Switch").
> > > 
> > > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > 
> > Thanks, applied now.
> > 
> > 
> > > +		if (strcmp(ncontrol->name, kctl.id.name) != 0)
> 
> Maybe better is to just use end char comparsion to save few CPU ticks:
> 
> if (kctl.id.name[sizeof(kctl.id.name)-2] != '\0' &&
>     ncontrol->name[sizeof(kctl.id.name)-1] != '\0')

No, this may cause segfault (and way too hacky to understand what's
the purpose of the code...)


Takashi

Re: [PATCH] ALSA: Warn when control names are truncated

[Mark Brown]

On Wed, Oct 29, 2008 at 03:51:40PM +0100, Jaroslav Kysela wrote:

> Maybe better is to just use end char comparsion to save few CPU ticks:

> if (kctl.id.name[sizeof(kctl.id.name)-2] != '\0' &&
>     ncontrol->name[sizeof(kctl.id.name)-1] != '\0')
> 	print_warning_here;

It will be but given that this only happens when controls are created
I'd be surprised if performance were a practical issue.

Re: [PATCH] ALSA: Warn when control names are truncated

[Jaroslav Kysela]

On Wed, 29 Oct 2008, Takashi Iwai wrote:

> At Wed, 29 Oct 2008 15:51:40 +0100 (CET),
> Jaroslav Kysela wrote:
> > 
> > On Wed, 29 Oct 2008, Takashi Iwai wrote:
> > 
> > > At Wed, 29 Oct 2008 14:40:30 +0000,
> > > Mark Brown wrote:
> > > > 
> > > > This is likely to confuse user interfaces since the end of the control
> > > > name is interpreted (eg, "Volume", "Switch").
> > > > 
> > > > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > > 
> > > Thanks, applied now.
> > > 
> > > 
> > > > +		if (strcmp(ncontrol->name, kctl.id.name) != 0)
> > 
> > Maybe better is to just use end char comparsion to save few CPU ticks:
> > 
> > if (kctl.id.name[sizeof(kctl.id.name)-2] != '\0' &&
> >     ncontrol->name[sizeof(kctl.id.name)-1] != '\0')
> 
> No, this may cause segfault

How? The first check in the fixed size variable initialized with zeros 
allocated on heap ensures that ncontrol->name string is at least 
sizeof(kctl.id.name) long. There is no possibility of segfault.

> (and way too hacky to understand what's the purpose of the code...)

The printk explains check nicely.

					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH] ALSA: Warn when control names are truncated

[Takashi Iwai]

At Wed, 29 Oct 2008 19:31:25 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Wed, 29 Oct 2008, Takashi Iwai wrote:
> 
> > At Wed, 29 Oct 2008 15:51:40 +0100 (CET),
> > Jaroslav Kysela wrote:
> > > 
> > > On Wed, 29 Oct 2008, Takashi Iwai wrote:
> > > 
> > > > At Wed, 29 Oct 2008 14:40:30 +0000,
> > > > Mark Brown wrote:
> > > > > 
> > > > > This is likely to confuse user interfaces since the end of the control
> > > > > name is interpreted (eg, "Volume", "Switch").
> > > > > 
> > > > > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > > > 
> > > > Thanks, applied now.
> > > > 
> > > > 
> > > > > +		if (strcmp(ncontrol->name, kctl.id.name) != 0)
> > > 
> > > Maybe better is to just use end char comparsion to save few CPU ticks:
> > > 
> > > if (kctl.id.name[sizeof(kctl.id.name)-2] != '\0' &&
> > >     ncontrol->name[sizeof(kctl.id.name)-1] != '\0')
> > 
> > No, this may cause segfault
> 
> How? The first check in the fixed size variable initialized with zeros 
> allocated on heap ensures that ncontrol->name string is at least 
> sizeof(kctl.id.name) long. There is no possibility of segfault.

Hm, OK, this would work indeed surprisingly.

> 
> > (and way too hacky to understand what's the purpose of the code...)
> 
> The printk explains check nicely.

Uh, no.  The problem is that you'll likely need to consider what
really the code does and even whether it's correct at the first
glance.  That's definitely bad for readability.

Since the code path is absolutely no fast path and a rarely used
path, there is no win at all in practice for such a nano
optimization.


Takashi