[pulseaudio-discuss] [PATCH] alsa-card: Don't free the modargs in pa_init.

[Tanu Kaskinen]

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