lkml.org 
[lkml]   [2007]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [ALSA] seq: resource leak fix and various code cleanups
    Hi Takashi-san,

    <quote sender="Takashi Iwai">
    > At Tue, 7 Aug 2007 16:40:48 +0800,
    > Eugene Teo wrote:
    > >
    > > This patch fixes:
    > > 1) a resource leak (CID: 1817)
    > > 2) various code cleanups
    [...]
    > > if (i >= SNDRV_SEQ_OSS_MAX_CLIENTS) {
    > > snd_printk(KERN_ERR "too many applications\n");
    > > - kfree(dp);
    > > - return -ENOMEM;
    > > + rc = -ENOMEM;
    > > + goto _error;
    >
    > This seems wrong. It goes before initializing dp->queue = -1, so it
    > screws up delete_seq_queue().

    You are right. The initialisations should come first.

    > > @@ -276,11 +281,13 @@ snd_seq_oss_open(struct file *file, int level)
    > > return 0;
    > >
    > > _error:
    > > - snd_seq_oss_synth_cleanup(dp);
    > > - snd_seq_oss_midi_cleanup(dp);
    > > - i = dp->queue;
    > > + snd_seq_oss_writeq_delete(dp->writeq);
    > > + snd_seq_oss_readq_delete(dp->readq);
    > > + delete_seq_queue(dp->queue);
    > > delete_port(dp);
    > > - delete_seq_queue(i);
    > > + snd_seq_oss_midi_cleanup(dp);
    > > + snd_seq_oss_synth_cleanup(dp);
    > > + kfree(dp);
    >
    > The order of these calls is important. Did you test it and confirm
    > that it has no side effects?

    Only snd_seq_oss_synth_cleanup() and snd_seq_oss_midi_cleanup() should
    be in order. The rest of the cleanup functions do not depend on one
    another. Thanks for your advice.

    Here's my second try:

    This patch fixes:
    1) a resource leak (CID: 1817)
    2) various code cleanups

    Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
    ---
    sound/core/seq/oss/seq_oss_init.c | 40 +++++++++++++++++++++--------------
    sound/core/seq/oss/seq_oss_writeq.c | 6 +++-
    2 files changed, 28 insertions(+), 18 deletions(-)

    diff --git a/sound/core/seq/oss/seq_oss_init.c b/sound/core/seq/oss/seq_oss_init.c
    index ca5a2ed..f26b751 100644
    --- a/sound/core/seq/oss/seq_oss_init.c
    +++ b/sound/core/seq/oss/seq_oss_init.c
    @@ -176,29 +176,31 @@ snd_seq_oss_open(struct file *file, int level)
    int i, rc;
    struct seq_oss_devinfo *dp;

    - if ((dp = kzalloc(sizeof(*dp), GFP_KERNEL)) == NULL) {
    + dp = kzalloc(sizeof(*dp), GFP_KERNEL);
    + if (!dp) {
    snd_printk(KERN_ERR "can't malloc device info\n");
    return -ENOMEM;
    }
    debug_printk(("oss_open: dp = %p\n", dp));

    + dp->cseq = system_client;
    + dp->port = -1;
    + dp->queue = -1;
    + dp->readq = NULL;
    + dp->writeq = NULL;
    +
    for (i = 0; i < SNDRV_SEQ_OSS_MAX_CLIENTS; i++) {
    if (client_table[i] == NULL)
    break;
    }
    +
    + dp->index = i;
    if (i >= SNDRV_SEQ_OSS_MAX_CLIENTS) {
    snd_printk(KERN_ERR "too many applications\n");
    - kfree(dp);
    - return -ENOMEM;
    + rc = -ENOMEM;
    + goto _error;
    }

    - dp->index = i;
    - dp->cseq = system_client;
    - dp->port = -1;
    - dp->queue = -1;
    - dp->readq = NULL;
    - dp->writeq = NULL;
    -
    /* look up synth and midi devices */
    snd_seq_oss_synth_setup(dp);
    snd_seq_oss_midi_setup(dp);
    @@ -211,14 +213,16 @@ snd_seq_oss_open(struct file *file, int level)

    /* create port */
    debug_printk(("create new port\n"));
    - if ((rc = create_port(dp)) < 0) {
    + rc = create_port(dp);
    + if (rc < 0) {
    snd_printk(KERN_ERR "can't create port\n");
    goto _error;
    }

    /* allocate queue */
    debug_printk(("allocate queue\n"));
    - if ((rc = alloc_seq_queue(dp)) < 0)
    + rc = alloc_seq_queue(dp);
    + if (rc < 0)
    goto _error;

    /* set address */
    @@ -235,7 +239,8 @@ snd_seq_oss_open(struct file *file, int level)
    /* initialize read queue */
    debug_printk(("initialize read queue\n"));
    if (is_read_mode(dp->file_mode)) {
    - if ((dp->readq = snd_seq_oss_readq_new(dp, maxqlen)) == NULL) {
    + dp->readq = snd_seq_oss_readq_new(dp, maxqlen);
    + if (dp->readq == NULL) {
    rc = -ENOMEM;
    goto _error;
    }
    @@ -253,7 +258,8 @@ snd_seq_oss_open(struct file *file, int level)

    /* initialize timer */
    debug_printk(("initialize timer\n"));
    - if ((dp->timer = snd_seq_oss_timer_new(dp)) == NULL) {
    + dp->timer = snd_seq_oss_timer_new(dp);
    + if (dp->timer == NULL) {
    snd_printk(KERN_ERR "can't alloc timer\n");
    rc = -ENOMEM;
    goto _error;
    @@ -276,11 +282,13 @@ snd_seq_oss_open(struct file *file, int level)
    return 0;

    _error:
    + snd_seq_oss_writeq_delete(dp->writeq);
    + snd_seq_oss_readq_delete(dp->readq);
    snd_seq_oss_synth_cleanup(dp);
    snd_seq_oss_midi_cleanup(dp);
    - i = dp->queue;
    delete_port(dp);
    - delete_seq_queue(i);
    + delete_seq_queue(dp->queue);
    + kfree(dp);

    return rc;
    }
    diff --git a/sound/core/seq/oss/seq_oss_writeq.c b/sound/core/seq/oss/seq_oss_writeq.c
    index 5c84956..2174248 100644
    --- a/sound/core/seq/oss/seq_oss_writeq.c
    +++ b/sound/core/seq/oss/seq_oss_writeq.c
    @@ -63,8 +63,10 @@ snd_seq_oss_writeq_new(struct seq_oss_devinfo *dp, int maxlen)
    void
    snd_seq_oss_writeq_delete(struct seq_oss_writeq *q)
    {
    - snd_seq_oss_writeq_clear(q); /* to be sure */
    - kfree(q);
    + if (q) {
    + snd_seq_oss_writeq_clear(q); /* to be sure */
    + kfree(q);
    + }
    }


    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2007-08-07 12:55    [W:0.027 / U:32.536 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site