-
Notifications
You must be signed in to change notification settings - Fork 124
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
oniguruma segfault #544
Comments
It seems to be a concurrency problem with oniguruma regexps. I'm not able to reproduce the error in my machine but created a binary with locks around regexp. This will be tested in a many cores machine. If this goes well I'll apply the changes in go-mysql-server and enry. |
We are going to implement another library and check if we have the same problems. |
Related info: k-takata/Onigmo#63 (comment) They say it is thread safe on >=6.0. What's the installed libonig.so version in the docker image? If the code runs on an ancient Debian I wouldn't be surprised if it is older. |
https://github.com/rubinius/oniguruma corresponds to 5.9.2 from 2013 https://github.com/kkos/oniguruma is 6.9, updated and maintained |
The machine where we saw this had libonig-dev 6.1.3-2 installed... but I think the bug happened with the statically linked binary published to GitHub releases. I'm not sure where we got that oniguruma version (cc @jfontan). If it was libonig-dev package from Travis, that was probably Ubuntu Trusty's: 5.9.1-1ubuntu1.1. |
@campoy I see you manually built some binaries on that machine too, do you know if panics happened with oniguruma-enabled builds when built directly from source? |
@smola the version was 6.8.2, from alpine. It was statically linked in a docker container. Still, oniguruma itself is not the problem, is the golang bindings. The buffers where to write intermediate results are the same for each compiled regexp ( We could fix that with one regexp node per thread, adding locking or changing the library to use different region and errorInfo each time it's called. I prefer the first one, less error prone. https://github.com/moovweb/rubex/blob/master/regex.go#L208 |
As discussed previously, I would also prefer one regexp node per thread. Just imagine thread contention in a 96 cores machine all waiting for a regexp lock... |
I do not think there were any crashes with the binary built from source, but not 100% sure. Also, 96 cores waiting for regexp lock is a great topic for Halloween 😨 |
It's a pity https://github.com/moovweb/rubex is archived so we cannot fix that for everyone... |
The text was updated successfully, but these errors were encountered: