[supernova] fix /b_fill handler

[catfact]

Purpose and Motivation

noticed that /b_fill commands didn't seem to be doing anything in supernova.

checking the source revealed that the sample index into the buffer data wasn't being incremented, so i added this.

i additionally added a bounds check for the index. surprisingly this doesn't happen in other command handlers like /b_set. is there a non-obvious mechanism for this elsewhere, or is it possible to smash memory with OOB arguments to these commands?

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist

• All previous tests are passing
• This PR is ready for review

i haven't added any unit tests, but of course would be happy to do so with a little more guidance.

[patrickdupuis]

Thanks for this PR!

I was able to reproduce the issue, and this PR fixes things.

Sorry for the silence. We are planning to have a Dev meeting next week to discuss 3.10 and future development.

[nhthn]

thanks! minor code style issues here: please move ++index into its own line, add a space between if (, and move break to a separate line

[brianlheim]

thanks, please make formatting changes as suggested by @snappizz , and rebase this on the 3.10 branch

[catfact]

ok no problem, i seem to be having some qt-related build issues with 3.10 but will hopefully sort it out presently.

[catfact]

@brianlheim @snappizz how's this look to yall?

(fwiw: i see many other violations of those formatting rules just in this source file. maybe uncrustify or clang-format is called for if the rules are serious. also documenting them would be nice.)

and, er... should i fix the other lack of bounds checking? OSC is an external API after all, one can't assume every script in the world is well behaved.

for example, i can confirm that this very evil sclang script will smash memory in supernova. usually this crashes it, and on at least one occasion i managed to take down the whole jack server.

Server.supernova;
s = Server.local;
s.boot;
s.waitForBoot { Routine {
	// some sound just to verify that we're running
	x = { SinOsc.ar([218, 222]) * 0.125 }.play(s);
	b = Buffer.alloc(s, 1);
	3.0.wait;
	n = 100000;
	n.do({ |i| s.sendMsg("/b_set", b.bufnum, i, 1.0.rand2); });
}.play };

oh and also - thanks for taking the time to review and for all your work on the project!

[brianlheim]

(fwiw: i see many other violations of those formatting rules just in this source file. maybe uncrustify or clang-format is called for if the rules are serious. also documenting them would be nice.)

both points are covered in our style guide which is referenced in our main CONTRIBUTING.md.

but yes, the codebase (which has seen many contributors) doesn't have a consistent style. i am planning to add a clang-format file and reformat everything during a quiet period, hopefully sometime in the next month or so.

and, er... should i fix the other lack of bounds checking? OSC is an external API after all, one can't assume every script in the world is well behaved.

yes, please! (in a separate PR) thank you for noticing that.

[brianlheim]

@bgola please double-check your indentation.

also, it looks like you didn't rebase this onto 3.10 properly. it should be easy to squash everything to a single commit and cherry-pick it. i think this should work:

git checkout 3.10 --detach
git cherry-pick -n 33dfb6c 466810b 685b182 e83acb1
git add --all
git commit -m "..."
git branch -f topic/fix-bfill-supernova
git checkout topic/fix-bfill-supernova
git push -f origin topic/fix-bfill-supernova

failing that you can just copy-paste the changes and create a new commit manually, just copy-paste instead of executing the cherry-pick command above.

[brianlheim]

@catfact did you mean to close this? what would you like to happen with this PR?

[catfact]

@brianlheim i am closing this b/c it is based on develop (per instructions in CONTRIBUTING - btw this did not include a working link to style guide when i first opened the PR; thanks for adding one in the meantime.)

but you have asked me to rebase on 3.10. i can't change the base of an existing PR so the rebased history looks wacky. oh, nvm, in fact i can. so i'll reopen this instead.

... aaand, nevermind again, somehow GH gets a little confused by that and the rebase becomes pointless. so, keeping this closed.

[bgola]

@bgola please double-check your indentation.

@brianlheim did you mean to tag me here? was there something wrong with my PR?

[brianlheim]

@bgola - oops, sorry, github autocompleted the wrong username!

... aaand, nevermind again, somehow GH gets a little confused by that and the rebase becomes pointless. so, keeping this closed.

I have found that in that situation, changing the base gets github to recalculate the commits. If the base is already the one you want you might have to change it twice. Pretty suboptimal on GH's part.