In the implementation of snd_korg1212_create() the allocated memory for
korg1212 is leaked in cases of error. Release korg1212 via
snd_korg1212_free() if either of these calls fail:
snd_korg1212_downloadDSPCode(), snd_pcm_new(), or snd_ctl_add().
Signed-off-by: Navid Emamdoost <[email protected]>
---
sound/pci/korg1212/korg1212.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
index 0d81eac0a478..e976e857d915 100644
--- a/sound/pci/korg1212/korg1212.c
+++ b/sound/pci/korg1212/korg1212.c
@@ -2367,8 +2367,10 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
mdelay(CARD_BOOT_DELAY_IN_MS);
- if (snd_korg1212_downloadDSPCode(korg1212))
+ if (snd_korg1212_downloadDSPCode(korg1212)) {
+ snd_korg1212_free(korg1212);
return -EBUSY;
+ }
K1212_DEBUG_PRINTK("korg1212: dspMemPhy = %08x U[%08x], "
"PlayDataPhy = %08x L[%08x]\n"
@@ -2383,8 +2385,11 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
korg1212->RoutingTablePhy, LowerWordSwap(korg1212->RoutingTablePhy),
korg1212->AdatTimeCodePhy, LowerWordSwap(korg1212->AdatTimeCodePhy));
- if ((err = snd_pcm_new(korg1212->card, "korg1212", 0, 1, 1, &korg1212->pcm)) < 0)
+ err = snd_pcm_new(korg1212->card, "korg1212", 0, 1, 1, &korg1212->pcm);
+ if (err < 0) {
+ snd_korg1212_free(korg1212);
return err;
+ }
korg1212->pcm->private_data = korg1212;
korg1212->pcm->private_free = snd_korg1212_free_pcm;
@@ -2398,8 +2403,10 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
for (i = 0; i < ARRAY_SIZE(snd_korg1212_controls); i++) {
err = snd_ctl_add(korg1212->card, snd_ctl_new1(&snd_korg1212_controls[i], korg1212));
- if (err < 0)
+ if (err < 0) {
+ snd_korg1212_free(korg1212);
return err;
+ }
}
snd_korg1212_proc_init(korg1212);
--
2.17.1
…
> +++ b/sound/pci/korg1212/korg1212.c
…
> @@ -2398,8 +2403,10 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>
> for (i = 0; i < ARRAY_SIZE(snd_korg1212_controls); i++) {
> err = snd_ctl_add(korg1212->card, snd_ctl_new1(&snd_korg1212_controls[i], korg1212));
> - if (err < 0)
> + if (err < 0) {
> + snd_korg1212_free(korg1212);
> return err;
> + }
I suggest to add a jump target according to the Linux coding style
so that duplicate exception handling code can be reduced.
return 0;
+free_korg:
+ snd_korg1212_free(korg1212);
+ return err;
}
Regards,
Markus
On Sun, 27 Oct 2019 20:12:04 +0100,
Navid Emamdoost wrote:
>
> In the implementation of snd_korg1212_create() the allocated memory for
> korg1212 is leaked in cases of error. Release korg1212 via
> snd_korg1212_free() if either of these calls fail:
> snd_korg1212_downloadDSPCode(), snd_pcm_new(), or snd_ctl_add().
This also leads to the double-free. The code path is after
snd_device_new() which has its own destructor callback.
thanks,
Takashi
>
> Signed-off-by: Navid Emamdoost <[email protected]>
> ---
> sound/pci/korg1212/korg1212.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
> index 0d81eac0a478..e976e857d915 100644
> --- a/sound/pci/korg1212/korg1212.c
> +++ b/sound/pci/korg1212/korg1212.c
> @@ -2367,8 +2367,10 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>
> mdelay(CARD_BOOT_DELAY_IN_MS);
>
> - if (snd_korg1212_downloadDSPCode(korg1212))
> + if (snd_korg1212_downloadDSPCode(korg1212)) {
> + snd_korg1212_free(korg1212);
> return -EBUSY;
> + }
>
> K1212_DEBUG_PRINTK("korg1212: dspMemPhy = %08x U[%08x], "
> "PlayDataPhy = %08x L[%08x]\n"
> @@ -2383,8 +2385,11 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> korg1212->RoutingTablePhy, LowerWordSwap(korg1212->RoutingTablePhy),
> korg1212->AdatTimeCodePhy, LowerWordSwap(korg1212->AdatTimeCodePhy));
>
> - if ((err = snd_pcm_new(korg1212->card, "korg1212", 0, 1, 1, &korg1212->pcm)) < 0)
> + err = snd_pcm_new(korg1212->card, "korg1212", 0, 1, 1, &korg1212->pcm);
> + if (err < 0) {
> + snd_korg1212_free(korg1212);
> return err;
> + }
>
> korg1212->pcm->private_data = korg1212;
> korg1212->pcm->private_free = snd_korg1212_free_pcm;
> @@ -2398,8 +2403,10 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>
> for (i = 0; i < ARRAY_SIZE(snd_korg1212_controls); i++) {
> err = snd_ctl_add(korg1212->card, snd_ctl_new1(&snd_korg1212_controls[i], korg1212));
> - if (err < 0)
> + if (err < 0) {
> + snd_korg1212_free(korg1212);
> return err;
> + }
> }
>
> snd_korg1212_proc_init(korg1212);
> --
> 2.17.1
>
> I suggest to add a jump target according to the Linux coding style
> so that duplicate exception handling code can be reduced.
Do you find any information interesting from the update suggestion
“ALSA: korg1212: Use common error handling code in two functions”?
https://lore.kernel.org/alsa-devel/[email protected]/
https://lore.kernel.org/patchwork/patch/851723/
https://lkml.org/lkml/2017/11/16/193
Regards,
Markus
> The code path is after snd_device_new() which has its own destructor callback.
Thanks for your reminder.
Can the properties of this programming interface be documented also together
with the function declaration for the safer handling of ALSA components?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/sound/kernel-api/writing-an-alsa-driver.rst?id=d6d5df1db6e9d7f8f76d2911707f7d5877251b02#n567
Can any more API information help to improve automatic source code analysis
around similar functions?
Regards,
Markus
On Mon, 28 Oct 2019 10:00:21 +0100,
Markus Elfring wrote:
>
> > The code path is after snd_device_new() which has its own destructor callback.
>
> Thanks for your reminder.
>
> Can the properties of this programming interface be documented also together
> with the function declaration for the safer handling of ALSA components?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/sound/kernel-api/writing-an-alsa-driver.rst?id=d6d5df1db6e9d7f8f76d2911707f7d5877251b02#n567
I can think of only adding some comment at each point mentioning that
the resource will be managed by the device destructor.
> Can any more API information help to improve automatic source code analysis
> around similar functions?
If anything we can add, let me know.
Takashi
>> Can the properties of this programming interface be documented also together
>> with the function declaration for the safer handling of ALSA components?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/sound/kernel-api/writing-an-alsa-driver.rst?id=d6d5df1db6e9d7f8f76d2911707f7d5877251b02#n567
>
> I can think of only adding some comment at each point mentioning that
> the resource will be managed by the device destructor.
* Which places will be corresponding update candidates?
* Would you like to choose the wording?
>> Can any more API information help to improve automatic source code analysis
>> around similar functions?
>
> If anything we can add, let me know.
Will any tags become more helpful also for the software documentation?
Regards,
Markus
On Mon, 28 Oct 2019 15:40:09 +0100,
Markus Elfring wrote:
>
> >> Can the properties of this programming interface be documented also together
> >> with the function declaration for the safer handling of ALSA components?
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/sound/kernel-api/writing-an-alsa-driver.rst?id=d6d5df1db6e9d7f8f76d2911707f7d5877251b02#n567
> >
> > I can think of only adding some comment at each point mentioning that
> > the resource will be managed by the device destructor.
>
> * Which places will be corresponding update candidates?
At each place where this kind of code is seen.
> * Would you like to choose the wording?
I myself don't mind as long as it's clearly understandable for human
being.
> >> Can any more API information help to improve automatic source code analysis
> >> around similar functions?
> >
> > If anything we can add, let me know.
>
> Will any tags become more helpful also for the software documentation?
It's my question.
thanks,
Takashi