opus - Jan 2011 - Memory leak when specifying invalid channels count

Hi

I minor issue I noticed when I passed an invalid 'channels' parameter
to celt_encoder_create() is that celt_encoder_init() doesn't free
the CELTEncoder ('st' parameter) when 'if (channels < 0 ||
channels > 2)'
evaluates to true.

Seems celt_encoder_init() should do like this instead:

CELTEncoder *celt_encoder_init(CELTEncoder *st, const CELTMode *mode, int
channels, int *error)
{
   if (channels < 0 || channels > 2)
   {
      if (error)
         *error = CELT_BAD_ARG;
      celt_free(st); /* ensure 'st' is deallocated */
      return NULL;
   }


Or maybe even better have celt_encoder_init() allocate the CELTEncoder so
it's only created if the input is valid.

-- Bjoern
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://lists.xiph.org/pipermail/opus/attachments/20110129/619de4b2/attachment-0002.htm

Hi Bjoern,

Thanks for reporting this. You're correct about the problem, although 
the solution is to change celt_encoder_create() because the array passed 
to celt_encoder_init() isn't guaranteed to be malloc()ed.

Cheers,

	Jean-Marc

On 11-01-29 11:36 AM, Bjoern Damstedt Rasmussen wrote:
> Hi
>
> I minor issue I noticed when I passed an invalid 'channels'
parameter
> to celt_encoder_create() is that celt_encoder_init() doesn't free
> the CELTEncoder ('st' parameter) when 'if (channels < 0 ||
channels >
> 2)' evaluates to true.
>
> Seems celt_encoder_init() should do like this instead:
>
> CELTEncoder *celt_encoder_init(CELTEncoder *st, const CELTMode *mode,
> int channels, int *error)
> {
>     if (channels < 0 || channels > 2)
>     {
>        if (error)
>           *error = CELT_BAD_ARG;
>        celt_free(st); /* ensure 'st' is deallocated */
>        return NULL;
>     }
>
>
> Or maybe even better have celt_encoder_init() allocate the CELTEncoder
> so it's only created if the input is valid.
>
> -- Bjoern
>
>
>
> _______________________________________________
> celt-dev mailing list
> celt-dev at xiph.org
> http://lists.xiph.org/mailman/listinfo/celt-dev

OK, this is fixed in git now. The bad news is that it goes with an API 
change (unrelated to the bug fix). The good news is that this will 
hopefully be the final API. The main thing that changed is that for 
"standard" modes, a CELTMode object is no longer necessary to create
an
encoder or decoder. For "non-standard" modes, the old behaviour can be
obtained using celt_encoder_create_custom() and simiar for the init() 
calls and decoder creation. Please try this out and report any problem 
you encounter.

Cheers,

	Jean-Marc

On 11-01-29 11:36 AM, Bjoern Damstedt Rasmussen wrote:
> Hi
>
> I minor issue I noticed when I passed an invalid 'channels'
parameter
> to celt_encoder_create() is that celt_encoder_init() doesn't free
> the CELTEncoder ('st' parameter) when 'if (channels < 0 ||
channels >
> 2)' evaluates to true.
>
> Seems celt_encoder_init() should do like this instead:
>
> CELTEncoder *celt_encoder_init(CELTEncoder *st, const CELTMode *mode,
> int channels, int *error)
> {
>     if (channels < 0 || channels > 2)
>     {
>        if (error)
>           *error = CELT_BAD_ARG;
>        celt_free(st); /* ensure 'st' is deallocated */
>        return NULL;
>     }
>
>
> Or maybe even better have celt_encoder_init() allocate the CELTEncoder
> so it's only created if the input is valid.
>
> -- Bjoern
>
>
>
> _______________________________________________
> celt-dev mailing list
> celt-dev at xiph.org
> http://lists.xiph.org/mailman/listinfo/celt-dev