Skip to content
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.

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

Stack size limit is not enforced #5871

Open
jamshark70 opened this issue Sep 14, 2022 · 5 comments
Open

Stack size limit is not enforced #5871

jamshark70 opened this issue Sep 14, 2022 · 5 comments
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library"

Comments

@jamshark70
Copy link
Contributor

jamshark70 commented Sep 14, 2022

Environment

  • SuperCollider version: 3.13dev
  • Operating system: Presumably any (I'm on Linux)
  • Other details (Qt version, audio driver, etc.): n/a

Steps to reproduce

(
~stackSizeTest = { |n = 1|
	var result = n * 2;
	if(n > 0) {
		~stackSizeTest.value(n - 1);
	};
	result  // disallow tail call optimization
};

r = Routine({
	~stackSizeTest.value(1000).postln;
}, stackSize: 200).play;
)

-> a Routine
2000

Expected vs. actual behavior

This example specifies a maximum stack depth of 200 for the thread.

It then requests 1000 levels of recursion.

Expected: Stack overflow error. (Or even, sclang crash here would be preferable to what I describe below.)

Actual: Recursion completes, returns n*2, and prints 2000.

Why this is a bad thing: We expect the stack size to protect against infinite recursion. But (at least in Linux), it doesn't. What happens instead is that every recursive iteration allocates a new stack frame (executeMethodWithKeys() line 894 and executeMethod() line 1055), and at this point, there is no stack depth check.

(There is a checkStackOverflow() function, which seems to be called only from a macro, dispatch_opcode. I suspect compiler flags may be disabling this, however. I tried to replace assert(checkStackOverflow(g, sp)); with if(!checkStackOverflow(g, sp)) { printf("Stack overflow\n"); std::abort(); }; and couldn't get the if branch to activate. That is, there appears to be an attempt to prevent stack overflow but at some point, accidentally, this may have been disabled.)

A riskier demonstration of the issue, then, is:

  1. Open your system's process monitor and filter to watch sclang.
  2. Change the Routine to call ~stackSizeTest.value(inf).
  3. Be ready to reboot the interpreter quickly.
  4. Run it.
  5. In the process monitor, watch sclang allocate multiple gigabytes of RAM within a few seconds. !!! !!! !!!
  6. At this point, you had better reboot the interpreter very quickly. Otherwise the OS will go into swap and your system may become completely unresponsive, in which case you would have no choice but to hard shutdown using the power switch.

I'm gonna gently suggest that sclang "expected behavior" should never include disabling the entire system and forcing a hard reboot.

This is a rare circumstance, and admittedly user error. But, part of the schtick of programming for fun is "go ahead, try it, you can't seriously break anything" but SC can't actually claim that -- you can break something by accidental infinite recursion. So I consider this a fairly high severity bug.

@jamshark70 jamshark70 added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" labels Sep 14, 2022
@dyfer
Copy link
Member

dyfer commented Sep 14, 2022

I agree that the boundaries should be enforced.

Off topic, but...
Why is it that a non-elevated process is able to take down the whole OS? In 2022? I tested ~stackSizeTest.value(inf) on macOS and I got a message that the system has run out of memory, with a dialog allowing to "force close" processes (with SuperCollider clocking in at 95GB of memory at the time - that included swap of course). I was able to kill the process there and come back to a working OS...

@jamshark70
Copy link
Contributor Author

Why is it that a non-elevated process is able to take down the whole OS?

That's a good point.

It could be about Linux user privileges. https://wiki.ubuntu.com/Audio/TheAudioGroup says "As a practical rule of thumb for Debian and Ubuntu systems, there should be no users in the audio group" but then "Exceptions: ... If you're running Jackd and need realtime privileges" which is every SC user as of now. Then: "The risk involved here is that if a malicious or badly written program makes use of these privileges, it can cause the entire system to lock up, or at least become very sluggish."

One reading of this is that, if SC requires JACK, then SC needs to avoid excessive memory-consumption scenarios.

That suggests another couple of "maybe" but unlikely solutions:

  1. Convince the JACK developers to figure out a way to do it without realtime privileges. (Here, I guess, if it were technically feasible, they might have already done it.)

  2. Switch scsynth to PipeWire sooner rather than later (if PipeWire doesn't require RT privileges).

@dyfer
Copy link
Member

dyfer commented Sep 14, 2022

I am vaguely aware of privileges and audio on Linux, however - why do we have to run sclang with the same privileges? Is it because scsynth inherits the privileges from the calling process? If so, it seems like it would be worthy to somehow address that, if that's at all possible.

@jamshark70
Copy link
Contributor Author

why do we have to run sclang with the same privileges?

AFAICS, TempoClock threads need RT priority in Linux (schedRun() and TempoClock::TempoClock()). But I don't know if they really need RT priority (maybe this was just the easiest way at the time the Linux port started).

@dyfer
Copy link
Member

dyfer commented Sep 15, 2022

TempoClock threads need RT priority in Linux

Ah, good to know. Maybe this is something to investigate then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library"
Projects
None yet
Development

No branches or pull requests

2 participants