On Mon, 2013-12-30 at 20:09 +0100, Peter Meerwald wrote:
> Hi,
>
> > > > The modargs are in both cases (a succesfull as well as a failed module
> > > > initialization) freed already in pa_done().
> > >
> > > the alsa module keeps a pointer to the modargs; hence, they MUST NOT be
> > > freed in the success case
> > >
> > > in the fail case, the pa_modargs_free() is redundant as you noted
> >
> > It's not entirely redundant. If we jump to fail before u->modargs has
> > been set, then there will be a memory leak if pa_modargs_free() isn't
> > called for ma (which I assume is why you added the pa_modargs_free()
> > calls there in the Coverity patch set).
>
> holy crap, alright
>
> so in the success case, no
> pa_modargs_free(ma);
> since modargs are free()'d in pa__done()
>
> in the fail case, we rely on pa__done() as well unless m->userdata ==
> NULL (this can happen due to the "ignore_dB" code), so
>
> fail:
> if (reserve)
> pa_reserve_wrapper_unref(reserve);
>
> if (ma && !m->userdata)
> pa_modargs_free(ma);
>
> pa__done(m);
>
> probably the better fix would be to move the following code in pa__init()
> if (pa_modargs_get_value_boolean(ma, "ignore_dB", &ignore_dB) < 0) {
> pa_log("Failed to parse ignore_dB argument.");
> goto fail;
> }
> a few lines down, i.e. before
> if ((u->alsa_card_index = snd_card_get_index(u->device_id)) < 0)
>
> any objections?
That's OK, but I think even better would be to avoid any possibility of
adding failable code between pa_modargs_new() and assigning the modargs
to u->modargs. The way to do that is to allocate userdata first, and
then do u->modargs = pa_modargs_new(). The ma variable won't be needed
at all.
--
Tanu
