Re: [PATCH] ALSA: sound/isa/gus: check the return value of kstrdup()

Re: [PATCH] ALSA: sound/isa/gus: check the return value of kstrdup()

[Xiaoke Wang]

Note: Compare with the last email, this one is using my full name.
kstrdup() returns NULL when some internal memory errors happen, it is
better to check the return value of it. Otherwise, we may not to be able
to catch some memory errors in time.

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
 sound/isa/gus/gus_mem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/isa/gus/gus_mem.c b/sound/isa/gus/gus_mem.c
index ff9480f..f8d915f 100644
--- a/sound/isa/gus/gus_mem.c
+++ b/sound/isa/gus/gus_mem.c
@@ -199,6 +199,8 @@ struct snd_gf1_mem_block *snd_gf1_mem_alloc(struct snd_gf1_mem * alloc, int owne
 		memcpy(&block.share_id, share_id, sizeof(block.share_id));
 	block.owner = owner;
 	block.name = kstrdup(name, GFP_KERNEL);
+	if (block.name == NULL)
+		return NULL;
 	nblock = snd_gf1_mem_xalloc(alloc, &block);
 	snd_gf1_mem_lock(alloc, 1);
 	return nblock;
@@ -237,13 +239,13 @@ int snd_gf1_mem_init(struct snd_gus_card * gus)
 		block.ptr = 0;
 		block.size = 1024;
 		block.name = kstrdup("InterWave LFOs", GFP_KERNEL);
-		if (snd_gf1_mem_xalloc(alloc, &block) == NULL)
+		if (block.name == NULL || snd_gf1_mem_xalloc(alloc, &block) == NULL)
 			return -ENOMEM;
 	}
 	block.ptr = gus->gf1.default_voice_address;
 	block.size = 4;
 	block.name = kstrdup("Voice default (NULL's)", GFP_KERNEL);
-	if (snd_gf1_mem_xalloc(alloc, &block) == NULL)
+	if (block.name == NULL || snd_gf1_mem_xalloc(alloc, &block) == NULL)
 		return -ENOMEM;
 #ifdef CONFIG_SND_DEBUG
 	snd_card_ro_proc_new(gus->card, "gusmem", gus, snd_gf1_mem_info_read);
-- 

Re: [PATCH] ALSA: sound/isa/gus: check the return value of kstrdup()

[Takashi Iwai]

On Mon, 13 Dec 2021 09:06:47 +0100,
Xiaoke Wang wrote:
> 
> Note: Compare with the last email, this one is using my full name.
> kstrdup() returns NULL when some internal memory errors happen, it is
> better to check the return value of it. Otherwise, we may not to be able
> to catch some memory errors in time.
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>

The patch again forgot about the proper error handling...
This will leave the mutex unbalanced.

Please be careful when writing this kind of fix at the next time.
Many code paths require the proper error handling, e.g. freeing the
rest memory or unlock/lock something.

In this particular case, snd_gf1_mem_lock(alloc, 1) is needed at the
first chunk.

And, the easiest way to fix would be to have a NULL check in
snd_gf1_mem_xalloc().  Then that will cover all callers by once.


thanks,

Takashi

> ---
>  sound/isa/gus/gus_mem.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/isa/gus/gus_mem.c b/sound/isa/gus/gus_mem.c
> index ff9480f..f8d915f 100644
> --- a/sound/isa/gus/gus_mem.c
> +++ b/sound/isa/gus/gus_mem.c
> @@ -199,6 +199,8 @@ struct snd_gf1_mem_block *snd_gf1_mem_alloc(struct snd_gf1_mem * alloc, int owne
>  		memcpy(&block.share_id, share_id, sizeof(block.share_id));
>  	block.owner = owner;
>  	block.name = kstrdup(name, GFP_KERNEL);
> +	if (block.name == NULL)
> +		return NULL;
>  	nblock = snd_gf1_mem_xalloc(alloc, &block);
>  	snd_gf1_mem_lock(alloc, 1);
>  	return nblock;
> @@ -237,13 +239,13 @@ int snd_gf1_mem_init(struct snd_gus_card * gus)
>  		block.ptr = 0;
>  		block.size = 1024;
>  		block.name = kstrdup("InterWave LFOs", GFP_KERNEL);
> -		if (snd_gf1_mem_xalloc(alloc, &block) == NULL)
> +		if (block.name == NULL || snd_gf1_mem_xalloc(alloc, &block) == NULL)
>  			return -ENOMEM;
>  	}
>  	block.ptr = gus->gf1.default_voice_address;
>  	block.size = 4;
>  	block.name = kstrdup("Voice default (NULL's)", GFP_KERNEL);
> -	if (snd_gf1_mem_xalloc(alloc, &block) == NULL)
> +	if (block.name == NULL || snd_gf1_mem_xalloc(alloc, &block) == NULL)
>  		return -ENOMEM;
>  #ifdef CONFIG_SND_DEBUG
>  	snd_card_ro_proc_new(gus->card, "gusmem", gus, snd_gf1_mem_info_read);
> -- 
> 

[PATCH] ALSA: sound/isa/gus: check the return value of kstrdup()

[xkernel]

kstrdup() returns NULL when some internal memory errors happen, it is
better to check the return value of it. Otherwise, we may not to be able
to catch some memory errors in time.

Signed-off-by: xkernel <xkernel.wang@foxmail.com>
---
 sound/isa/gus/gus_mem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/isa/gus/gus_mem.c b/sound/isa/gus/gus_mem.c
index ff9480f..f8d915f 100644
--- a/sound/isa/gus/gus_mem.c
+++ b/sound/isa/gus/gus_mem.c
@@ -199,6 +199,8 @@ struct snd_gf1_mem_block *snd_gf1_mem_alloc(struct snd_gf1_mem * alloc, int owne
 		memcpy(&block.share_id, share_id, sizeof(block.share_id));
 	block.owner = owner;
 	block.name = kstrdup(name, GFP_KERNEL);
+	if (block.name == NULL)
+		return NULL;
 	nblock = snd_gf1_mem_xalloc(alloc, &block);
 	snd_gf1_mem_lock(alloc, 1);
 	return nblock;
@@ -237,13 +239,13 @@ int snd_gf1_mem_init(struct snd_gus_card * gus)
 		block.ptr = 0;
 		block.size = 1024;
 		block.name = kstrdup("InterWave LFOs", GFP_KERNEL);
-		if (snd_gf1_mem_xalloc(alloc, &block) == NULL)
+		if (block.name == NULL || snd_gf1_mem_xalloc(alloc, &block) == NULL)
 			return -ENOMEM;
 	}
 	block.ptr = gus->gf1.default_voice_address;
 	block.size = 4;
 	block.name = kstrdup("Voice default (NULL's)", GFP_KERNEL);
-	if (snd_gf1_mem_xalloc(alloc, &block) == NULL)
+	if (block.name == NULL || snd_gf1_mem_xalloc(alloc, &block) == NULL)
 		return -ENOMEM;
 #ifdef CONFIG_SND_DEBUG
 	snd_card_ro_proc_new(gus->card, "gusmem", gus, snd_gf1_mem_info_read);
--