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

no_stack_overflow_on_drop tests fail on FreeBSD #967

Closed
bjorn3 opened this issue Mar 21, 2023 · 4 comments · Fixed by #968
Closed

no_stack_overflow_on_drop tests fail on FreeBSD #967

bjorn3 opened this issue Mar 21, 2023 · 4 comments · Fixed by #968

Comments

@bjorn3
Copy link
Member

bjorn3 commented Mar 21, 2023

What version of regex are you using?

a9b2e02

Describe the bug at a high level.

The no_stack_overflow_on_drop tests crash on FreeBSD with SIGBUS followed by SIGSEGV. This happens while spawning the thread. I am pretty sure it overflows the stack. 1k of stack seem to be way too little for spawning a thread on FreeBSD. The smallest stack size for which I was able to make it work is 8k. Anything below either crashes or hangs.

What are the steps to reproduce the behavior?

Run cargo test -p regex-syntax on FreeBSD.

What is the actual behavior?

(gdb) run
Starting program: /tmp/cirrus-ci-build/download/regex/target/debug/deps/regex_syntax-872800c174f9b4f7 
warning: Could not load shared library symbols for [vdso].
Do you need "set solib-search-path" or "set sysroot"?
[New LWP 115642 of process 30733]

Thread 2 received signal SIGBUS, Bus error.
[Switching to LWP 115642 of process 30733]
0x00000008019775b4 in find_symdef (symnum=1266, refobj=refobj@entry=0x801993008, defobj_out=defobj_out@entry=0x7fffdfffd0c0, flags=flags@entry=1, cache=cache@entry=0x0, lockstate=lockstate@entry=0x7fffdfffd058) at /usr/src/libexec/rtld-elf/rtld.c:2031
2031        if (ELF_ST_BIND(ref->st_info) != STB_LOCAL) {
(gdb) bt
#0  0x00000008019775b4 in find_symdef (symnum=1266, refobj=refobj@entry=0x801993008, defobj_out=defobj_out@entry=0x7fffdfffd0c0, 
    flags=flags@entry=1, cache=cache@entry=0x0, lockstate=lockstate@entry=0x7fffdfffd058) at /usr/src/libexec/rtld-elf/rtld.c:2031
#1  0x00000008019773fc in _rtld_bind (obj=0x801993008, reloff=14856) at /usr/src/libexec/rtld-elf/rtld.c:1028
#2  0x0000000801972fbd in _rtld_bind_start () at /usr/src/libexec/rtld-elf/amd64/rtld_start.S:121
#3  0x0000000801bbcf9e in malloc_mutex_trylock_final (mutex=<optimized out>)
    at /usr/src/contrib/jemalloc/include/jemalloc/internal/mutex.h:159
#4  malloc_mutex_lock (tsdn=0x801e67090, mutex=<optimized out>) at /usr/src/contrib/jemalloc/include/jemalloc/internal/mutex.h:218
#5  tsd_add_nominal (tsd=0x801e67090) at jemalloc_tsd.c:92
#6  __je_tsd_state_set (tsd=tsd@entry=0x801e67090, new_state=new_state@entry=0 '\000') at jemalloc_tsd.c:193
#7  0x0000000801bbd2e7 in __je_tsd_fetch_slow (tsd=0x801e67090, minimal=<optimized out>) at jemalloc_tsd.c:285
#8  0x0000000801b6c34b in tsd_fetch_impl (init=true, minimal=false)
    at /usr/src/contrib/jemalloc/include/jemalloc/internal/tsd.h:355
#9  tsd_fetch () at /usr/src/contrib/jemalloc/include/jemalloc/internal/tsd.h:381
#10 imalloc (sopts=<optimized out>, dopts=<optimized out>) at jemalloc_jemalloc.c:2256
#11 __je_malloc_default (size=64) at jemalloc_jemalloc.c:2293
#12 0x00000008019bc284 in _thr_attr_init (attr=0x7fffdfffd290) at /usr/src/lib/libthr/thread/thr_attr.c:365
#13 0x0000000001713bf7 in std::sys::unix::thread::guard::current () at sysroot_src/library/std/src/sys/unix/thread.rs:861
#14 0x00000000014f9b62 in std::thread::Builder::spawn_unchecked_::{{closure}} ()
    at /tmp/cirrus-ci-build/download/sysroot/sysroot_src/library/std/src/thread/mod.rs:523
#15 0x00000000014b7b9d in core::ops::function::FnOnce::call_once{{vtable.shim}} ()
    at /tmp/cirrus-ci-build/download/sysroot/sysroot_src/library/core/src/ops/function.rs:250
#16 0x0000000001686fee in <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once ()
    at /tmp/cirrus-ci-build/download/sysroot/sysroot_src/library/alloc/src/boxed.rs:1988
#17 0x0000000001686f8b in <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once ()
    at /tmp/cirrus-ci-build/download/sysroot/sysroot_src/library/alloc/src/boxed.rs:1988
#18 0x000000000164ffed in std::sys::unix::thread::Thread::new::thread_start ()
    at sysroot_src/library/std/src/sys/unix/thread.rs:108
#19 0x00000008019be83a in thread_start (curthread=0x802212700) at /usr/src/lib/libthr/thread/thr_create.c:292

What is the expected behavior?

The test passes.

BurntSushi added a commit that referenced this issue Mar 21, 2023
This test works by spinning up a thread with an atypically small stack
size, parsing a regex into an Ast and then dropping it. We use a small
stack size such that *if the Ast didn't have a custom Drop impl*, then
its default recursive Drop impl would overflow the stack. (If we don't
use a smaller stack size, then the default on some platforms is usually
quite large and might require a much larger Ast to provoke a failure.)

It turns out that the stack size we were using was quite tiny, and too
tiny for some platforms such as FreeBSD. We therefore increase it a
little bit, but not too much.

We do the same for the corresponding test for the custom Drop impl for
Hir.

Fixes #967
@BurntSushi
Copy link
Member

Hmmm. The test is definitely not particularly robust. The problem with choosing a large stack size is that you also then have to build a bigger expression. For example, on my (Linux) system, the test passes:

$ cargo t --manifest-path regex-syntax/Cargo.toml --lib ast::tests
   Compiling regex-syntax v0.6.28 (/home/andrew/code/rust/regex/regex-syntax)
    Finished test [unoptimized + debuginfo] target(s) in 1.83s
     Running unittests src/lib.rs (target/debug/deps/regex_syntax-cfadee16848a08de)

running 1 test
test ast::tests::no_stack_overflow_on_drop ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 137 filtered out; finished in 0.00s

But that isn't quite the point of the test. Because the test also depends on the fact that if you remove the custom Drop impl on Ast, then the test fails (with a stack overflow):

     Running unittests src/lib.rs (target/debug/deps/regex_syntax-cfadee16848a08de)

running 1 test

thread '<unknown>' has overflowed its stack
fatal runtime error: stack overflow
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/home/andrew/code/rust/regex/target/debug/deps/regex_syntax-cfadee16848a08de 'ast::tests'` (signal: 6, SIGABRT: process abort signal)

So okay, I bumped the stack size to 8K and the test does indeed still fail. I then bumped it to 16K and the test still fails. Since that feels comfortably above the minimum that worked for you, I think that sounds okay to me? (It actually fails with 31K too, but not with 32K. So 16K seems like a good number.)

I put #968 up to fix this (including the corresponding test for Hir, which I presume is probably also failing for you).

@BurntSushi
Copy link
Member

Also, can I just say, I'm so fucking excited for Cranelift. It is amazing how far y'all are!

BurntSushi added a commit that referenced this issue Mar 21, 2023
This test works by spinning up a thread with an atypically small stack
size, parsing a regex into an Ast and then dropping it. We use a small
stack size such that *if the Ast didn't have a custom Drop impl*, then
its default recursive Drop impl would overflow the stack. (If we don't
use a smaller stack size, then the default on some platforms is usually
quite large and might require a much larger Ast to provoke a failure.)

It turns out that the stack size we were using was quite tiny, and too
tiny for some platforms such as FreeBSD. We therefore increase it a
little bit, but not too much.

We do the same for the corresponding test for the custom Drop impl for
Hir.

Fixes #967
@BurntSushi
Copy link
Member

This is fixed in regex 1.7.2 and regex-syntax 0.6.29.

@bjorn3
Copy link
Member Author

bjorn3 commented Mar 21, 2023

Thanks! Updated in rust-lang/rustc_codegen_cranelift@ae0a22c and confirmed that it works on FreeBSD.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Apr 11, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [regex](https://github.com/rust-lang/regex) | dependencies | patch | `1.7.1` -> `1.7.3` |

---

### Release Notes

<details>
<summary>rust-lang/regex</summary>

### [`v1.7.3`](https://github.com/rust-lang/regex/blob/HEAD/CHANGELOG.md#&#8203;173-2023-03-24)

[Compare Source](rust-lang/regex@1.7.2...1.7.3)

\==================
This is a small release that fixes a bug in `Regex::shortest_match_at` that
could cause it to panic, even when the offset given is valid.

Bug fixes:

-   [BUG #&#8203;969](rust-lang/regex#969):
    Fix a bug in how the reverse DFA was called for `Regex::shortest_match_at`.

### [`v1.7.2`](https://github.com/rust-lang/regex/blob/HEAD/CHANGELOG.md#&#8203;172-2023-03-21)

[Compare Source](rust-lang/regex@1.7.1...1.7.2)

\==================
This is a small release that fixes a failing test on FreeBSD.

Bug fixes:

-   [BUG #&#8203;967](rust-lang/regex#967):
    Fix "no stack overflow" test which can fail due to the small stack size.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xOS4wIiwidXBkYXRlZEluVmVyIjoiMzUuNDAuMiJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1827
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants