After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 772186 - fdkaac: misc memory related fixes
fdkaac: misc memory related fixes
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.9.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-09-29 13:30 UTC by Vincent Penquerc'h
Modified: 2016-09-30 07:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix buffer leak (957 bytes, patch)
2016-09-29 13:34 UTC, Vincent Penquerc'h
committed Details | Review
avoid memory corruption on error path (1.06 KB, patch)
2016-09-29 13:34 UTC, Vincent Penquerc'h
none Details | Review
fix access to freed memory (1.57 KB, patch)
2016-09-29 13:35 UTC, Vincent Penquerc'h
committed Details | Review
avoid memory corruption on error path (2.22 KB, patch)
2016-09-29 14:03 UTC, Vincent Penquerc'h
committed Details | Review

Description Vincent Penquerc'h 2016-09-29 13:30:41 UTC
fdkaac: misc memory related fixes
Comment 1 Vincent Penquerc'h 2016-09-29 13:34:27 UTC
Created attachment 336518 [details] [review]
fix buffer leak
Comment 2 Vincent Penquerc'h 2016-09-29 13:34:54 UTC
Created attachment 336519 [details] [review]
avoid memory corruption on error path
Comment 3 Vincent Penquerc'h 2016-09-29 13:35:19 UTC
Created attachment 336520 [details] [review]
fix access to freed memory
Comment 4 Sebastian Dröge (slomo) 2016-09-29 13:58:31 UTC
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 5 Sebastian Dröge (slomo) 2016-09-29 13:59:54 UTC
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.
Comment 6 Vincent Penquerc'h 2016-09-29 14:03:39 UTC
Created attachment 336521 [details] [review]
avoid memory corruption on error path
Comment 7 Vincent Penquerc'h 2016-09-29 14:05:28 UTC
(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.
Comment 8 Sebastian Dröge (slomo) 2016-09-29 14:06:59 UTC
(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()?
Comment 9 Vincent Penquerc'h 2016-09-29 14:08:51 UTC
Anywhere after _DecodeFrame would be OK. Here is just a convenient place.
Comment 10 Vincent Penquerc'h 2016-09-29 14:10:04 UTC
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 11 Sebastian Dröge (slomo) 2016-09-29 14:10:18 UTC
Comment on attachment 336520 [details] [review]
fix access to freed memory

Ok, please merge with a comment about *why* :)
Comment 12 Vincent Penquerc'h 2016-09-29 14:18:57 UTC
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