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 780331 - interleave segfaults with more than 64 channels
interleave segfaults with more than 64 channels
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other Linux
: Normal normal
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-20 21:27 UTC by Douglas Bagnall
Modified: 2017-03-31 11:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
possibly a fix (1.55 KB, patch)
2017-03-20 22:40 UTC, Douglas Bagnall
none Details | Review
patch series (4.64 KB, patch)
2017-03-23 11:42 UTC, Douglas Bagnall
committed Details | Review
Patch for plugins-bad/audiomixer/audiointerleave (2.56 KB, patch)
2017-03-31 11:05 UTC, Douglas Bagnall
committed Details | Review

Description Douglas Bagnall 2017-03-20 21:27:07 UTC
Since 7ae7f657fae542f06ba4c22d65a2084f5a4a3a5d (interleave: interleave samples following the Default Channel Ordering), if you try to interleave more than 64 channels, you stomp all over memory. In my case I see:

Thread 21 "wavparse18:sink" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff97fff700 (LWP 23185)]
0x00007ffff1e2f3de in gst_pad_push_event (pad=0x4e0000004d, event=0x7fff90004df0) at gstpad.c:5339

because a pad pointer has been overwritten with 0x4d and 0x4e.


The problem looks simple enough: default_ordering_map below is a fixed size but channels is not constrained:

gst_interleave_channel_positions_to_mask (GValueArray * positions,
    gint default_ordering_map[64], guint64 * mask)
{
  //...

  channels = positions->n_values;

  //..
  /* sort the default ordering map according to the position order */
  for (i = 0; i < channels; i++) {
    default_ordering_map[i] = i;
  }


I am not sure of the correct solution. Personally I need at least 200 channels but (as far as I know) I don't care about the default ordering.


This was found on Ubuntu 16.04 with the packaged Gstreamer 1.8.3. I don't think it has been fixed since.
Comment 1 Douglas Bagnall 2017-03-20 21:33:57 UTC
would something like

 for (i = 0; i < MIN(channels, 64); i++) {
    default_ordering_map[i] = i;
  }

be sufficient?

CC'ing Antonio Ospite.
Comment 2 Douglas Bagnall 2017-03-20 22:40:19 UTC
Created attachment 348366 [details] [review]
possibly a fix

(In reply to Douglas Bagnall from comment #1)
> would something like
> 
>  for (i = 0; i < MIN(channels, 64); i++) {
>     default_ordering_map[i] = i;
>   }

Obviously not, of course. Nor does the attached patch seem quite right. Should we even be anywhere near here with >64 channels? Can't we just say the channel mask is inapplicable and skip the whole lot?

(With previous version I recall having to set the channel mask to one thing or another to navigate through this thicket).
Comment 3 Sebastian Dröge (slomo) 2017-03-21 11:23:07 UTC
The solution would be to make channel-mask=0 and just pass through channels as is without any reordering or so.
Comment 4 Sebastian Dröge (slomo) 2017-03-21 11:29:42 UTC
Comment on attachment 348366 [details] [review]
possibly a fix

See above, you would just take the channels in the order they are without reordering or anything and make channel-mask=0
Comment 5 Douglas Bagnall 2017-03-23 11:42:23 UTC
Created attachment 348562 [details] [review]
patch series

Patch 1/2 fixes the ordering_map bounds problem and sets channel-mask = 0 when there are more than 64 channels.

Is that the kind of thing you meant Slomo?

Patch 2/2 addresses the hypothetical situation where the ordering map is not filled out because self->channel_positions == NULL or some other reason, which is indicated externally in gst_interleave_set_channel_positions by setting the channel_mask to 0 but forgotten internally when gst_interleave_collected comes to use the uninitialised array (I am not certain that this situation is actually possible).
Comment 6 Sebastian Dröge (slomo) 2017-03-31 09:20:37 UTC
Comment on attachment 348562 [details] [review]
patch series

Looks both good to me, but can you also make the same patch for the audiointerleave element in gst-plugins-bad/gst/audiomixer? The interleave element is deprecated by that one, audiointerleave generally works better.
Comment 7 Douglas Bagnall 2017-03-31 11:05:35 UTC
Created attachment 349037 [details] [review]
Patch for plugins-bad/audiomixer/audiointerleave

(In reply to Sebastian Dröge (slomo) from comment #6)
> Looks both good to me, but can you also make the same patch for the
> audiointerleave element in gst-plugins-bad/gst/audiomixer? The interleave
> element is deprecated by that one, audiointerleave generally works better.

OK, that's good to know. 

https://bug780331.bugzilla-attachments.gnome.org/attachment.cgi?id=348562 is actually 2 patches, the first of which fixes the bug, and the other covers the possible use of an uninitialised mapping.

This patch is the first of those two, fixing the case with >64 channels. I am not sure if the second one is applicable.
Comment 8 Sebastian Dröge (slomo) 2017-03-31 11:11:03 UTC
Comment on attachment 348562 [details] [review]
patch series

commit a9f26c2a14928cf666bbbea0d0320a7681bdabb3
Author: Douglas Bagnall <douglas@halo.gen.nz>
Date:   Fri Mar 24 00:11:13 2017 +1300

    interleave: avoid using uninitialised ordering_map
    
    If self->channel_positions == NULL (which seems unlikely),
    self->default_channels_ordering_map will be used unintialised.
    
    We avoid that by keeping track of the channel_mask, which is set when
    the ordering map is initialised.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=780331

commit c08d719453e1b6347531fa0d050cb02043d2d6f4
Author: Douglas Bagnall <douglas@halo.gen.nz>
Date:   Thu Mar 23 23:56:31 2017 +1300

    interleave: don't overflow channel map with >64 channels
    
    When there are more than 64 channels, we don't want to exceed the
    bounds of the ordering_map buffer, and in these cases we don't want to
    rempa at all. Here we avoid doing that.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=780331
Comment 9 Sebastian Dröge (slomo) 2017-03-31 11:12:25 UTC
commit e83573fd8d232a0b6defe99ca4ee28c4cbef0ff8
Author: Douglas Bagnall <douglas@halo.gen.nz>
Date:   Fri Mar 31 23:40:05 2017 +1300

    audiointerleave: don't overflow channel map with >64 channels
    
    When there are more than 64 channels, we don't want to exceed the
    bounds of the ordering_map buffer, and in these cases we don't want to
    remap at all. Here we avoid doing that.
    
    Based on a patch originally for plugins-good/interleave in
    https://bugzilla.gnome.org/show_bug.cgi?id=780331