GNOME Bugzilla – Bug 780331
interleave segfaults with more than 64 channels
Last modified: 2017-03-31 11:12:57 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.
would something like for (i = 0; i < MIN(channels, 64); i++) { default_ordering_map[i] = i; } be sufficient? CC'ing Antonio Ospite.
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).
The solution would be to make channel-mask=0 and just pass through channels as is without any reordering or so.
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
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 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.
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 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
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