GNOME Bugzilla – Bug 772186
fdkaac: misc memory related fixes
Last modified: 2016-09-30 07:06:12 UTC
Created attachment 336518 [details] [review] fix buffer leak
Created attachment 336519 [details] [review] avoid memory corruption on error path
Created attachment 336520 [details] [review] fix access to freed memory
Comment on attachment 336519 [details] [review] avoid memory corruption on error path Or g_new(guint16, self->decode_buffer_size) Change it to that and merge
Comment on attachment 336520 [details] [review] fix access to freed memory The ref shouldn't be needed. The buffer is valid for the whole scope of the function. Only the later unmap could be useful.
Created attachment 336521 [details] [review] avoid memory corruption on error path
(In reply to Sebastian Dröge (slomo) from comment #5) > Comment on attachment 336520 [details] [review] [review] > fix access to freed memory > > The ref shouldn't be needed. The buffer is valid for the whole scope of the > function. Only the later unmap could be useful. It appears that _finish_frame will unref it, a bit too early for us.
(In reply to Vincent Penquerc'h from comment #7) > (In reply to Sebastian Dröge (slomo) from comment #5) > > Comment on attachment 336520 [details] [review] [review] [review] > > fix access to freed memory > > > > The ref shouldn't be needed. The buffer is valid for the whole scope of the > > function. Only the later unmap could be useful. > > It appears that _finish_frame will unref it, a bit too early for us. It indeed does, yes. Until when do we have to keep the reference though? Why is it safe to only keep it until end of handle_frame()?
Anywhere after _DecodeFrame would be OK. Here is just a convenient place.
I mean, there is an early out on error, so I'd have to unmap at two places. But I can do that instead.
Comment on attachment 336520 [details] [review] fix access to freed memory Ok, please merge with a comment about *why* :)
commit ce59031b10efcf025c820704d8b8b9f6d215a85c Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Thu Sep 29 14:32:15 2016 +0100 fdkaacenc: fix accessing freed memory The buffer data is not always copied in _Fill, and will be read in _DecodeFrame. We unmap at the end of the function, whether we get there via failure or early out, and keep a ref to the buffer to ensure we can use it to unmap the memory even after _finish_frame is called, as it unrefs the buffer. Note that there is an access beyond the allocated buffer, which is only apparent when playing from souphttpsrc (ie, not from filesrc). This appears to be a bug in the bit reading code in libfdkaac AFAICT. https://bugzilla.gnome.org/show_bug.cgi?id=772186 commit 58bb21c463dfdb956e1a6811d345c556c9d95b17 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Thu Sep 29 14:31:37 2016 +0100 fdkaacdec: avoid memory corruption on decoding error The buffer size is expected to be in multiples of the sample size, not in bytes. https://bugzilla.gnome.org/show_bug.cgi?id=772186 commit 95de5bf19395132c67c9b8a2ef93e2a92226e1b2 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Thu Sep 29 14:29:46 2016 +0100 fdkaacenc: fix buffer leak https://bugzilla.gnome.org/show_bug.cgi?id=772186