diff mbox series

[2/2] ALSA: gus: Fix memory leaks at error paths in memory allocations

Message ID 20211213132444.22385-2-tiwai@suse.de (mailing list archive)
State New, archived
Headers show
Series [1/2] ALSA: gus: Fix erroneous memory allocation | expand

Commit Message

Takashi Iwai Dec. 13, 2021, 1:24 p.m. UTC
When snd_gf1_mem_xalloc() returns NULL, the current code still leaves
the formerly allocated block.name string but returns an error
immediately.  This patch covers the all callers to deal with the
release of leftover name strings in the error paths.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/isa/gus/gus_mem.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Jaroslav Kysela Dec. 13, 2021, 2:02 p.m. UTC | #1
On 13. 12. 21 14:24, Takashi Iwai wrote:
> When snd_gf1_mem_xalloc() returns NULL, the current code still leaves
> the formerly allocated block.name string but returns an error
> immediately.  This patch covers the all callers to deal with the
> release of leftover name strings in the error paths.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

It may be easier to pass the name to snd_gf1_mem_xalloc() - the code flow is more readable:

diff --git a/sound/isa/gus/gus_mem.c b/sound/isa/gus/gus_mem.c
index ff9480f249fe..cd087267b3ea 100644
--- a/sound/isa/gus/gus_mem.c
+++ b/sound/isa/gus/gus_mem.c
@@ -25,14 +25,22 @@ void snd_gf1_mem_lock(struct snd_gf1_mem * alloc, int xup)
  }
  
  static struct snd_gf1_mem_block *snd_gf1_mem_xalloc(struct snd_gf1_mem * alloc,
-					       struct snd_gf1_mem_block * block)
+					       struct snd_gf1_mem_block * block,
+					       const char * _name)
  {
  	struct snd_gf1_mem_block *pblock, *nblock;
+	char *name;
  
+	name = kstrdup(_name, GFP_KERNEL);
+	if (name == NULL)
+		return NULL
  	nblock = kmalloc(sizeof(struct snd_gf1_mem_block), GFP_KERNEL);
-	if (nblock == NULL)
+	if (nblock == NULL) {
+		kfree(name);
  		return NULL;
+	}
  	*nblock = *block;
+	nblock->name = name;
  	pblock = alloc->first;
  	while (pblock) {
  		if (pblock->ptr > nblock->ptr) {
@@ -198,8 +206,7 @@ struct snd_gf1_mem_block *snd_gf1_mem_alloc(struct snd_gf1_mem * alloc, int owne
  	if (share_id != NULL)
  		memcpy(&block.share_id, share_id, sizeof(block.share_id));
  	block.owner = owner;
-	block.name = kstrdup(name, GFP_KERNEL);
-	nblock = snd_gf1_mem_xalloc(alloc, &block);
+	nblock = snd_gf1_mem_xalloc(alloc, &block, name);
  	snd_gf1_mem_lock(alloc, 1);
  	return nblock;
  }
@@ -236,14 +243,12 @@ int snd_gf1_mem_init(struct snd_gus_card * gus)
  	if (gus->gf1.enh_mode) {
  		block.ptr = 0;
  		block.size = 1024;
-		block.name = kstrdup("InterWave LFOs", GFP_KERNEL);
-		if (snd_gf1_mem_xalloc(alloc, &block) == NULL)
+		if (snd_gf1_mem_xalloc(alloc, &block, "InterWave LFOs") == 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 (snd_gf1_mem_xalloc(alloc, &block, "Voice default (NULL's)") == NULL)
  		return -ENOMEM;
  #ifdef CONFIG_SND_DEBUG
  	snd_card_ro_proc_new(gus->card, "gusmem", gus, snd_gf1_mem_info_read);

						Jaroslav
Takashi Iwai Dec. 13, 2021, 2:06 p.m. UTC | #2
On Mon, 13 Dec 2021 15:02:46 +0100,
Jaroslav Kysela wrote:
> 
> On 13. 12. 21 14:24, Takashi Iwai wrote:
> > When snd_gf1_mem_xalloc() returns NULL, the current code still leaves
> > the formerly allocated block.name string but returns an error
> > immediately.  This patch covers the all callers to deal with the
> > release of leftover name strings in the error paths.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> It may be easier to pass the name to snd_gf1_mem_xalloc() - the code flow is more readable:
(snip)

Thanks, looks like a good idea.

Since the kstrdup() NULL check fix was already applied, I'll resubmit
with the new one.


Takashi
diff mbox series

Patch

diff --git a/sound/isa/gus/gus_mem.c b/sound/isa/gus/gus_mem.c
index 5e3ff3137dd7..1509e3e5d30e 100644
--- a/sound/isa/gus/gus_mem.c
+++ b/sound/isa/gus/gus_mem.c
@@ -204,6 +204,8 @@  struct snd_gf1_mem_block *snd_gf1_mem_alloc(struct snd_gf1_mem * alloc, int owne
 		return NULL;
 	}
 	nblock = snd_gf1_mem_xalloc(alloc, &block);
+	if (!nblock)
+		kfree(block.name);
 	snd_gf1_mem_lock(alloc, 1);
 	return nblock;
 }
@@ -241,14 +243,22 @@  int snd_gf1_mem_init(struct snd_gus_card * gus)
 		block.ptr = 0;
 		block.size = 1024;
 		block.name = kstrdup("InterWave LFOs", GFP_KERNEL);
-		if (block.name == NULL || snd_gf1_mem_xalloc(alloc, &block) == NULL)
+		if (block.name == NULL)
 			return -ENOMEM;
+		if (snd_gf1_mem_xalloc(alloc, &block) == NULL) {
+			kfree(block.name);
+			return -ENOMEM;
+		}
 	}
 	block.ptr = gus->gf1.default_voice_address;
 	block.size = 4;
 	block.name = kstrdup("Voice default (NULL's)", GFP_KERNEL);
-	if (block.name == NULL || snd_gf1_mem_xalloc(alloc, &block) == NULL)
+	if (block.name == NULL)
 		return -ENOMEM;
+	if (snd_gf1_mem_xalloc(alloc, &block) == NULL) {
+		kfree(block.name);
+		return -ENOMEM;
+	}
 #ifdef CONFIG_SND_DEBUG
 	snd_card_ro_proc_new(gus->card, "gusmem", gus, snd_gf1_mem_info_read);
 #endif