Fix stack overflow crash when BPH is set to guess and signal is lost#16
Fix stack overflow crash when BPH is set to guess and signal is lost#16colincoleman wants to merge 4 commits into
Conversation
Reject period estimates in compute_update() when bph == 0 and sigma == 0 (insufficient statistical confidence). Prevents bogus large periods from reaching VLA allocations on the 544KB computing thread stack. Tests added for bug condition and preservation of existing behavior.
|
I can confirm from testing at home that the app no longer crashes when bph is set to guess after this change. |
On macOS, secondary threads created with pthread_create(..., NULL, ...) get a 512KB stack instead of RLIMIT_STACK. The VLAs in compute_amplitude() and compute_parameters() can reach ~372KB for slow watches (max_period=31752 at 12000 BPH, 44.1kHz), exceeding that limit with call frames included. Query RLIMIT_STACK and pass it via pthread_attr_setstacksize, falling back to 8MB if getrlimit fails or returns 0/RLIM_INFINITY. On Linux this is a no-op since RLIMIT_STACK is already the pthread default. Fixes: xyzzy42#15
|
|
||
| struct rlimit rl; | ||
| size_t stack_size; | ||
| if (getrlimit(RLIMIT_STACK, &rl) == 0 && rl.rlim_cur != 0 && rl.rlim_cur != RLIM_INFINITY) { |
There was a problem hiding this comment.
Does this work in Windows? I was under the impression getrlimit() wasn't available in Windows and so adding this will break the windows build.
This also isn't going to help if the rlimit value is too small and will possible cause a problem if the rlimit value is too large. And if someone has rlimit set to unlimited on macos, it also wouldn't help.
I think a better design would be to get the default stack size and see if that is large enough. Increase the stack size if the default is less than something like 2 MB.
|
|
||
| if (ps[step].ready && ps[step].sigma < ps[step].period / 10000) { | ||
| if (ps[step].ready && ps[step].sigma < ps[step].period / 10000 | ||
| && (c->actv->bph != 0 || ps[step].sigma > 0)) { |
There was a problem hiding this comment.
I don't think the bph check is appropriate here. There's no reason the test for an acceptable fit at the given window size to be different if the bph was guessed or not. It just complicates the algorithm by creating different behavior where an auto bph and a manual bph of the exact same value produce different results later.
It should be the case that once the bph is guessed correctly, it makes no difference anywhere else in the algorithm.
I also think requiring sigma > 0 isn't right either. If single cycle cross-correlation peaks are never acceptable, then they should be rejected when finding the period.
|
Did you look at the tests? It doesn't test anything. They are just piles of AI slop that blather on about testing and a bunch of total nonsense and print out something that looks like a test result. |
…fault Two review fixes for the guess-mode crash: - Reject the period in compute_period() when it is confirmed by fewer than two cycles, instead of returning it with sigma = 0. A single-cycle cross-correlation peak has no statistical confidence and was the source of the bogus periods seen with noise in guess mode. This applies uniformly, so a guessed bph behaves the same as the identical manual value, and the bph/sigma special case in compute_update() is removed. - Size the computing thread stack from the pthread default rather than getrlimit(RLIMIT_STACK): read the default and raise it to a 2 MB minimum only when smaller. Avoids getrlimit (unavailable on Windows) and the too-small / unlimited rlimit cases; a no-op where the default already suffices. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous tests/test_bug_condition.c and test_preservation.c reimplemented copies of the logic and asserted against those copies, testing nothing in the real program; compiled binaries were committed alongside them. Remove them. Add tests/test_guess_no_signal.c, which runs the actual detection (analyze_processing_data -> process -> compute_period) on deterministic noise in guess mode and requires every analysis window to reject it - the precondition for the crash this PR fixes. Wired into "make check". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Updated to follow the same path for guessed and set BPH — single-cycle peaks are now rejected in compute_period() rather than gated later. Also dropped getrlimit: it reads the pthread default stack size and bumps to a 2 MB minimum only if smaller, so it won't break the Windows build. And replaced the AI-slop tests with one that runs the real detection on noise in guess mode (sorry for not checking those!). |
2 participants
Addresses #15. Reject period estimates in compute_update() when bph == 0 and sigma == 0 (insufficient statistical confidence). Prevents bogus large periods from reaching VLA allocations on the 544KB computing thread stack.
Tests added for bug condition and preservation of existing behavior.