| Message ID | tencent_1E3950293AC22395ACFE99404C985D738309@qq.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ALSA: sound/isa/gus: check the return value of kstrdup() | expand |
On Mon, 13 Dec 2021 11:52:32 +0100, Xiaoke Wang wrote: > > 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> > --- > Changes in v2: > - add the proper error handling. > Thanks for the suggestions from Takashi Iwai. In the future, I'll pay > more attention to what he mentioned. Thanks, now I applied as is. BTW, when you look at the code carefully, you'll find that the memory leaks may occur at the second and the third chunk in your patch. It's not a fault of this change, but it's a bug in the original code. Actually there are two issues -- one is about the incorrect return value from snd_gf1_mem_xalloc() and another is the missing kfree of block.name. Will submit two fix patches as a follow up. Takashi > --- > sound/isa/gus/gus_mem.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/sound/isa/gus/gus_mem.c b/sound/isa/gus/gus_mem.c > index ff9480f..4c691db 100644 > --- a/sound/isa/gus/gus_mem.c > +++ b/sound/isa/gus/gus_mem.c > @@ -199,6 +199,10 @@ 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) { > + snd_gf1_mem_lock(alloc, 1); > + return NULL; > + } > nblock = snd_gf1_mem_xalloc(alloc, &block); > snd_gf1_mem_lock(alloc, 1); > return nblock; > @@ -237,13 +241,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); > -- >
diff --git a/sound/isa/gus/gus_mem.c b/sound/isa/gus/gus_mem.c index ff9480f..4c691db 100644 --- a/sound/isa/gus/gus_mem.c +++ b/sound/isa/gus/gus_mem.c @@ -199,6 +199,10 @@ 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) { + snd_gf1_mem_lock(alloc, 1); + return NULL; + } nblock = snd_gf1_mem_xalloc(alloc, &block); snd_gf1_mem_lock(alloc, 1); return nblock; @@ -237,13 +241,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);
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> --- Changes in v2: - add the proper error handling. Thanks for the suggestions from Takashi Iwai. In the future, I'll pay more attention to what he mentioned. --- sound/isa/gus/gus_mem.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) --