VBAP: fix 2D loudspeaker-set parse stride (heap underflow / crash on free)#435
VBAP: fix 2D loudspeaker-set parse stride (heap underflow / crash on free)#435oudeis01 wants to merge 1 commit into
Conversation
…ree) VBAPSpeakerArray writes 10 floats per 2D set (2 speaker indices + 4 inverse + 4 forward matrix), but VBAP_Ctor parsed 2D at stride 6, miscounting the sets and reading matrix floats as speaker indices. That yields ls[i]==0, so final_gs[ls[i]-1] writes final_gs[-1] -- a heap underflow that corrupts the realtime pool and crashes scsynth when the VBAP unit is freed (VBAP_Dtor -> RTFree). Parse 2D like 3D: stride (dim*dim*2)+dim, and read the forward matrix for 2D as well. Confirmed with AddressSanitizer (overflow gone, valid in-bounds final_gs writes) and a churn test (no crash). Also fixes x_set_matx being uninitialized in the 2D gains_modified branch.
|
Hi @muellmusik, would you be able to review this by any chance? |
Sure. Might take me a bit. |
|
@oudeis01 Have you done a real world test to confirm that everything sounds correct? |
|
Hi @muellmusik thanks for asking. and yes, here is a minimal repro plus measured gains. Same SC code, two builds. Boot Tested:
Gains seem wrong even before the crash. Holding a constant input at fixed azimuths and reading the per-channel output (channel value == VBAP gain), spread = 0:
(speaker map: ch0=30°, ch1=-30°, ch2=0°, ch3=100°, ch4=-100°, ch5=135°, ch6=-135°) Across a full -180°..180° sweep (spreads 0/15/45): patched holds constant power Σg² = 1.0 everywhere and matches the analytic VBAP reference (re-derived from the set inverse matrices) to ~1e-7; stock has wide silent dead zones (Σg² → 0) and routes energy to wrong/out-of-range channels. Side-by-side per-channel gain curves (patched left, stock right; dotted = analytic reference): 
One disclaimer: I discovered the issue first in a studio setup with 12ch rig, but my current troubleshooting setup is a 2ch home rig, so I have not yet listened to all 7 channels. I will do the full multichannel listening this week and report back. |
|
Follow-up to my note: TL;DR I've now run the 7-channel listening test on the studio rig, and the patched build sounds correct across all channels. Setup
Patched (this PR)
Stock (current release), same session, VBAP.so swapped back
|
| counter = (numvals - 2) / ((unit->x_dimension * unit->x_dimension*2) + unit->x_dimension); | ||
| if(unit->x_dimension == 2) | ||
| counter = (numvals - 2) / ((unit->x_dimension * unit->x_dimension) + unit->x_dimension); | ||
| counter = (numvals - 2) / ((unit->x_dimension * unit->x_dimension*2) + unit->x_dimension); |
There was a problem hiding this comment.
If the counter math is the same regardless of the dimension, the ifs aren't needed. Same with the change below.
However, making the calculations identical regardless of the dimension seems odd... why were they different in the first place? @muellmusik
There was a problem hiding this comment.
Agreed on the cleanup:
after this fix the 2D and 3D counter expressions are identical
(numvals-2)/(dim*dim*2 + dim)
and the if(dim==2 || dim==3) below is effectively unconditional since dim is only ever 2 or 3, so both can collapse to a single branch. I kept them split here only to mirror the existing structure and keep the diff minimal; happy to squash them, but thinking it would be better after I clarify the bug sufficiently.
On "why were they different in the first place" :
as far as I can trace, they were not, until #376 ("Update channel limits and VBAP 1.0.3.2", 2025-10-21).
That commit added the forward (non-inverted) matrix to the 2D layout on the class side
vbap.sc: list_length = amount * 6 + 2 -> amount * 10 + 2, plus the new mat[] write and its computation in calc_2D_inv_tmatrix
and on the C side it updated the 3D reader to the wider stride and added the 3D forward-matrix read, but the 2D reader was left at the old /6 stride with the forward-matrix read still gated by if(dim==3). So the 2D/3D asymmetry looks like the 2D read path simply not being updated alongside the 2D write path in #376, rather than an intentional difference.
I agree taht it would be much clearer if @muellmusik would confirm the intended 2D layout and whether the 2D reader was meant to match the 3D one here. If so, collapsing the branches as @mtmccrea suggests is the natural finish.
|
Here's a straightforward way to plot speaker gains using the minimal repro settings you provided above: (
s.waitForBoot {
~azis = [30, -30, 0, 100, -100, 135, -135]; // 7-speaker 2D ring
~buf = Buffer.loadCollection(s, VBAPSpeakerArray.new(2, ~azis).getSetsAndMatrices);
}
)
(
{
VBAP.ar(7,
DC.ar(1), // signal
~buf.bufnum, // VBAP data
Line.kr(-180,180,1), // position
0, 0);
}.plot(1).superpose_(false);
)For me (macOS, SC 3.14), the provided example does not break/crash: 
Aside: In case it's relevant, what are data are you plotting in your above plots? Is it from SC output? looks like matplotlib/python? |
|
Hello @mtmccrea, thank you for the code and the plot. I would like to break down my answer to two: On my figures: you are right that I used python for the plotting, though the capture is done in SC, not python. I ran a headless sclang that feeds On why it does not break for you: I think it is a release skew, not a platform dependent luck/unluck. The 2D buffer layout changed in #376.
With the 10-float layout read at a 6-float stride, the first set parses fine and every later set reads matrix floats as speaker indices, which is what produces the dead zones here and, on this Linux box, a heap-buffer-overflow write into the gains array (ASAN flags it at VBAP_Ctor, gains[-1]). #376 first shipped in sc3-plugins 3.14.0 (2026-05-17), the previous release 3.13.0 predates it. So a pre-3.14.0 install has I note that you mention your SC version but not the sc3-plugins version (my box is sclang 3.14.1 with sc3-plugins 3.14.0). (
var azis = [30, -30, 0, 100, -100, 135, -135];
var raw = VBAPSpeakerArray.new(2, azis).getSetsAndMatrices;
("raw.size = " ++ raw.size ++ " sets/10 = " ++ ((raw.size - 2) / 10)).postln;
)I prediction is that, if you get a Btw, on 3.14.0 here it crashes with your exact code: the |
|
This will reproduce the dead-zone / crash behaviour of what I have reported in the PR. (
var here, azis, azGrid, spreads, settle;
var server, buf, synth, csvRows, t0, log, stride, nSets, raw;
var gotReply, lastReply, sumsqLog;
here = thisProcess.nowExecutingPath.dirname;
azis = [30, -30, 0, 100, -100, 135, -135]; // canonical 7-speaker 2D ring (the one that triggers the bug)
azGrid = (-180, -175 .. 180); // 73 probe directions
spreads = [0, 15, 45];
settle = 0.14; // > VBAP slew, < anything slow
t0 = Main.elapsedTime;
log = {|m| ("[" ++ (Main.elapsedTime - t0).round(0.01).asString ++ "] " ++ m).postln };
csvRows = List["spread,azimuth,g0,g1,g2,g3,g4,g5,g6,sumsq"];
sumsqLog = List[]; // (spread0 only) sum of squares per azimuth, for the verdict
server = Server.default;
server.options.numOutputBusChannels = 16;
server.options.memSize = 262144;
server.options.numBuffers = 1024;
server.options.blockSize = 64;
log.("SuperCollider " ++ Main.version);
server.waitForBoot {
Routine {
var arr;
log.("server booted, default plugins (your installed VBAP.so)");
arr = VBAPSpeakerArray.new(2, azis);
raw = arr.getSetsAndMatrices;
// --- self-report the layout (the macOS vs Linux discriminator) ---------
// pre-#376 sclang writes 6 floats/set (raw.size = amount*6 + 2),
// #376+ sclang writes 10 floats/set (raw.size = amount*10 + 2).
stride = if(((raw.size - 2) % 10) == 0) { 10 }
{ if(((raw.size - 2) % 6) == 0) { 6 } { -1 } };
nSets = if(stride > 0) { ((raw.size - 2) / stride).asInteger } { -1 };
"".postln;
"=== layout self-report =====================================".postln;
("raw.size (numvals) = " ++ raw.size).postln;
("detected stride = " ++ stride ++ " floats/set"
++ if(stride == 10) { " (#376+ : sc3-plugins >= 3.14.0)" }
{ if(stride == 6) { " (pre-#376 : sc3-plugins <= 3.13.x)" } { " (UNRECOGNISED)" } }).postln;
("real loudspeaker sets = " ++ nSets).postln;
"============================================================".postln;
"".postln;
buf = Buffer.loadCollection(server, raw);
server.sync;
SynthDef(\vbapProbe, {|azimuth = 0, spread = 0, snap = 0|
var sig = DC.ar(1);
var pan = VBAP.ar(7, sig, buf.bufnum, azimuth, 0, spread);
SendReply.kr(Trig1.kr(snap, 0.01), '/gains', A2K.kr(pan));
// no Out: silent capture
}).add;
server.sync;
gotReply = false; lastReply = nil;
OSCdef(\g, {|msg| lastReply = msg[3..9]; gotReply = true }, '/gains');
synth = Synth(\vbapProbe, [\azimuth, 9999, \spread, 0], server); // odd start so first set sticks
server.sync; 0.2.wait;
spreads.do {|sp|
log.("sweep spread=" ++ sp);
azGrid.do {|az|
var g, ssq;
synth.set(\spread, sp, \azimuth, az);
settle.wait;
gotReply = false;
synth.set(\snap, 1);
(0..40).do {|tries|
if(gotReply.not and: { server.serverRunning }) { 0.01.wait };
};
synth.set(\snap, 0);
if(server.serverRunning.not) {
"".postln;
("*** SERVER DIED at spread=" ++ sp ++ " az=" ++ az
++ " -> this is the crash; you are on the broken layout ***").postln;
File.use(here +/+ "vbap_gains_repro.csv", "w", {|f|
csvRows.do {|r| f.write(r ++ "\n") } });
("wrote partial " ++ (here +/+ "vbap_gains_repro.csv")).postln;
// 0.exit; // uncomment for headless
nil.yield;
};
g = if(gotReply) { lastReply } { Array.fill(7, { \nan }) };
ssq = if(gotReply) { g.collect {|x| x * x }.sum } { \nan };
if(sp == 0) { sumsqLog.add(ssq) };
csvRows.add(
([sp, az] ++ g ++ [ssq]).collect {|x|
if(x == \nan) { "nan" } { x.asString }
}.join(",")
);
};
};
// --- write CSV next to this file (no out/ dir needed) ------------------
File.use(here +/+ "vbap_gains_repro.csv", "w", {|f|
csvRows.do {|r| f.write(r ++ "\n") } });
log.("wrote " ++ (here +/+ "vbap_gains_repro.csv") ++ " (" ++ (csvRows.size - 1) ++ " rows)");
// --- in-language verdict (no Python needed) ----------------------------
if(sumsqLog.size > 0) {
var clean = sumsqLog.select {|x| x.isNumber };
var dead = clean.count {|x| x < 0.5 }; // constant-power should keep this ~1.0
"".postln;
"=== verdict (spread=0, constant-power check) ===============".postln;
("sum(g^2) min / max = " ++ clean.minItem.round(0.001) ++ " / " ++ clean.maxItem.round(0.001)
++ " (ideal = 1.0 everywhere)").postln;
("dead-ish directions = " ++ dead ++ " / " ++ clean.size ++ " (sum(g^2) < 0.5)").postln;
if(dead > 0) {
"=> DEAD ZONES PRESENT: this install mis-parses the 2D sets (the bug).".postln;
} {
"=> no dead zones: this install parses the 2D sets correctly.".postln;
};
"============================================================".postln;
};
synth.free;
log.("DONE (server left running; synth freed)");
// 0.exit; // uncomment for a headless one-shot
}.play;
};
) |
4 participants
Summary
For 2D speaker setups,
VBAP_Ctorparses the loudspeaker-set buffer with the wrong per-set stride.VBAPSpeakerArray.getSetsAndMatrices(choose_ls_tupletsinvbap.sc) writes 10 floats per 2D set (2 speaker indices + 4 inverse-matrix + 4 forward-matrix), but the Ctor reads 6 per set and computescounter = (numvals-2)/((dim*dim)+dim). It therefore miscounts the sets and, from the second set on, reads matrix floats as speaker indices. The bad index can be 0, sofinal_gs[ls[i]-1]writesfinal_gs[-1]a heap-buffer underflow. In therealtime
RTAllocpool this silently corrupts an adjacent block, surfacing as a SIGSEGV when the VBAP unit is freed (VBAP_Dtor->RTFree).Reproduction (SC 3.14.1, sc3-plugins 3.14.0)
Root cause (AddressSanitizer)
Building VBAP with
RTAlloc/RTFreeredirected tomalloc/free(so each allocation gets ASan redzones; the stock SC pool is one big malloc that hides intra-pool overflow) pinpoints it:Instrumented print right after
vbap(g,ls,unit):numvals = 72 = 7*10 + 2confirms 10 floats per 2D set, but the Ctor computescounter = (72-2)/6 = 11(should be 7).Fix
Parse 2D the same way as 3D (stride
(dim*dim*2)+dim, and read the forward matrix for 2D too). After the fix:x_lsset_amount = 7,ls0=2 ls1=3(valid, in-bounds), ASan reports zero errors, and the reproduction above plus a churn test (60 spawn/free, spread 0/15/45) run clean.This also fixes a second latent issue: the
gains_modifiedbranch usesx_set_matx[winner_set]for 2D as well, but 2D never populatedx_set_matx, so that path previously read uninitialized memory.Note
The
// needs to check that numOutputs and x_ls_amount match!!TODO atVBAP.cpp:599is a separate latent bounds issue (whenVBAP.ar's numChans < speaker count) and is not the cause of this crash (the repro uses 7 == 7); a defensive clamp there would be a reasonable follow-up, though out of scope here.