question about sound/pci/ctxfi/ctpcm.c

question about sound/pci/ctxfi/ctpcm.c

[Julia Lawall]

The file sound/pci/ctxfi/ctpcm.c contains the functions 
ct_pcm_playback_open and ct_pcm_capture_open that contain the following 
pattern of code:

        runtime->private_data = apcm;
	...
	if (err < 0) {
                kfree(apcm);
                return err;
        }

I wonder if this leaves a dangling pointer to apcm in runtime?
The function ct_atc_pcm_free_substream on the other hand does set the 
private_data field to NULL after freeing apcm.  But perhaps there is 
something in the calling context of open that ensures that if the open 
function fails, the private_data field of runtime will never be used?  

thanks,
julia

Re: question about sound/pci/ctxfi/ctpcm.c

[Clemens Ladisch]

Julia Lawall wrote:
> The file sound/pci/ctxfi/ctpcm.c contains the functions 
> ct_pcm_playback_open and ct_pcm_capture_open that contain the following 
> pattern of code:
> 
>         runtime->private_data = apcm;
> 	...
> 	if (err < 0) {
>                 kfree(apcm);
>                 return err;
>         }
> 
> I wonder if this leaves a dangling pointer to apcm in runtime?

The runtime structure contains data that is valid only while the
substream is open; it is allocated by the ALSA framework before
calling the open callback, and deallocated after calling the close
callback (or if the open callback fails).

> The function ct_atc_pcm_free_substream on the other hand does set the 
> private_data field to NULL after freeing apcm.

This is superfluous.

> But perhaps there is something in the calling context of open that
> ensures that if the open function fails, the private_data field of
> runtime will never be used?

If the open callback fails, the close callback will not be called.
However, the runtime->private_free callback, if set, will be called.

So there is indeed a dangling pointer.


Regards,
Clemens

Re: question about sound/pci/ctxfi/ctpcm.c

[Julia Lawall]

On Tue, 9 Nov 2010, Clemens Ladisch wrote:

> Julia Lawall wrote:
> > The file sound/pci/ctxfi/ctpcm.c contains the functions 
> > ct_pcm_playback_open and ct_pcm_capture_open that contain the following 
> > pattern of code:
> > 
> >         runtime->private_data = apcm;
> > 	...
> > 	if (err < 0) {
> >                 kfree(apcm);
> >                 return err;
> >         }
> > 
> > I wonder if this leaves a dangling pointer to apcm in runtime?
> 
> The runtime structure contains data that is valid only while the
> substream is open; it is allocated by the ALSA framework before
> calling the open callback, and deallocated after calling the close
> callback (or if the open callback fails).
> 
> > The function ct_atc_pcm_free_substream on the other hand does set the 
> > private_data field to NULL after freeing apcm.
> 
> This is superfluous.
> 
> > But perhaps there is something in the calling context of open that
> > ensures that if the open function fails, the private_data field of
> > runtime will never be used?
> 
> If the open callback fails, the close callback will not be called.
> However, the runtime->private_free callback, if set, will be called.
> 
> So there is indeed a dangling pointer.

Thanks for the explanation.  One option would be to move the 
initializations downwards, since the intervening calls that use runtime 
don't use those fields.  The other would be to set provate_data to NULL 
after doing the free.  Any preference?

thanks,
julia

Re: question about sound/pci/ctxfi/ctpcm.c

[Clemens Ladisch]

Julia Lawall wrote:
> [...]  One option would be to move the
> initializations downwards, since the intervening calls that use runtime
> don't use those fields.  The other would be to set provate_data to NULL
> after doing the free.  Any preference?

I notice that ct_atc_pcm_free_substream does not expect private_data to
be NULL, so only the first option would work.  Furthermore, apcm->timer
must not be NULL, so the initialization must be moved to the end of the
function.


Regards,
Clemens

Re: question about sound/pci/ctxfi/ctpcm.c

[Julia Lawall]

On Tue, 9 Nov 2010, Clemens Ladisch wrote:

> Julia Lawall wrote:
> > [...]  One option would be to move the
> > initializations downwards, since the intervening calls that use runtime
> > don't use those fields.  The other would be to set provate_data to NULL
> > after doing the free.  Any preference?
> 
> I notice that ct_atc_pcm_free_substream does not expect private_data to
> be NULL, so only the first option would work.  Furthermore, apcm->timer
> must not be NULL, so the initialization must be moved to the end of the
> function.

Thanks.  I will send a patch later today.

julia