Bjoern Damstedt Rasmussen
2011-Jan-29 16:36 UTC
[CELT-dev] 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
Jean-Marc Valin
2011-Jan-29 20:17 UTC
[CELT-dev] Memory leak when specifying invalid channels count
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
Jean-Marc Valin
2011-Jan-30 17:46 UTC
[CELT-dev] Memory leak when specifying invalid channels count
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