Skip to content

VBAP: fix 2D loudspeaker-set parse stride (heap underflow / crash on free) #435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dyfer merged 3 commits into from
Aug 26, 2026
Merged

VBAP: fix 2D loudspeaker-set parse stride (heap underflow / crash on free) #435

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3798e9
VBAP: fix 2D loudspeaker-set parse stride (heap underflow, crash on f…
oudeis01 Jun 4, 2026
2e25ba0
vbap.cpp: fix internal matrix handling in 2D vs. 3D case
mtmccrea Jun 21, 2026
80e5571
help: VBAP: add warning about speaker gaps, clarifications
mtmccrea Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions source/VBAPUGens/VBAP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ static void VBAP_Ctor(VBAP* unit)
if(unit->x_dimension == 3)
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);

@mtmccrea mtmccrea Jun 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@oudeis01 oudeis01 Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@muellmusik muellmusik Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, making the calculations identical regardless of the dimension seems odd... why were they different in the first place? @muellmusik

That's a good question! As this was a port from the PD update, I'm really not sure tbh, but I strongly suspect it came from the PD code.

unit->x_lsset_amount=counter;

if(counter<=0){
Expand Down Expand Up @@ -674,7 +674,7 @@ static void VBAP_Ctor(VBAP* unit)
unit->x_set_inv_matx[setpointer][i]=buf->data[datapointer++];
/* post("%d",deb++); */
}
if(unit->x_dimension == 3){
if(unit->x_dimension == 2 || unit->x_dimension == 3){
for(i=0; i < unit->x_dimension*unit->x_dimension; i++){
unit->x_set_matx[setpointer][i]=buf->data[datapointer++];

Expand Down