[alsa-devel] [PATCH] alsactl: sprintf to snprintf prevent buffer overflow

[Takashi Iwai]

At Wed,  1 May 2013 11:30:26 -0500,
Doug Goldstein wrote:
> 
> sprintf() is a bit dangerous unless you explicitly know your type size
> and want to keep it in sync always. Its safer to just use snprintf() and
> ensure your string doesn't overflow and is NULL terminated.
> 
> Signed-off-by: Doug Goldstein <cardoe at cardoe.com>
> ---
> This was written to fix the buffer overflow crash in alsactl with the
> 1.0.27 release, which is documented in Gentoo bug #468160 [1]. The
> issue is actually already fixed upstream by
> 95788fea25c1a59985828d4b91af0772d077600b however the fix just changes
> the size and continues to use sprintf() which IMHO is still dangerous.
> The attached patch will fix the issue in a safer manner for good.
> 
> [1] https://bugs.gentoo.org/show_bug.cgi?id=468160

Thanks, applied it now.

Though, the change in the second part is obviously superfluous
(i.e. not "optimal").  But stupidity isn't bad so that one can be even
more stupid sometimes :)


Takashi


> ---
>  alsactl/lock.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/alsactl/lock.c b/alsactl/lock.c
> index fce208b..587a109 100644
> --- a/alsactl/lock.c
> +++ b/alsactl/lock.c
> @@ -53,9 +53,9 @@ static int state_lock_(const char *file, int lock, int timeout)
>  	lck.l_len = 11;
>  	lck.l_pid = 0;
>  	if (lock) {
> -		sprintf(lcktxt, "%10li\n", (long)getpid());
> +		snprintf(lcktxt, sizeof(lcktxt), "%10li\n", (long)getpid());
>  	} else {
> -		sprintf(lcktxt, "%10s\n", "");
> +		snprintf(lcktxt, sizeof(lcktxt), "%10s\n", "");
>  	}
>  	while (fd < 0 && timeout-- > 0) {
>  		fd = open(nfile, O_RDWR);
> -- 
> 1.8.2.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>