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

Segmentation fault when application stops (Ruby 3.2.2, tcmalloc or jemalloc) #292

Closed
ukolovda opened this issue Sep 1, 2023 · 17 comments
Closed
Labels
bug/crash Bugs specific to crashes, segfaults, etc. Gem can be installed though. bug jemalloc The issue is related to the use of jemalloc

Comments

@ukolovda
Copy link

ukolovda commented Sep 1, 2023

Segfault occured when I stop the docker container.

I'm not sure that is Mini_racer trouble, but call stack is in it.

Dump is in the attachment.

MiniRacer SegFault dump.txt

@tisba
Copy link
Collaborator

tisba commented Nov 5, 2023

Hey @ukolovda 👋

Can you provide us with some more information? Most importantly: Can you reproduce it? And if so, can you provide a minimal example?

@jasoncodes
Copy link

Here’s a small Dockerfile which reproduces this issue on both amd64 and arm64:

FROM ruby:3.2.2-slim-bookworm

RUN \
  apt-get update --yes && \
  DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
    build-essential libjemalloc2 && \
  ln -s /usr/lib/*-linux-gnu/libjemalloc.so.2 /usr/lib/libjemalloc.so.2
ENV LD_PRELOAD=/usr/lib/libjemalloc.so.2

RUN gem install mini_racer

CMD ruby -r mini_racer -e 'p MiniRacer::Context.new.eval("1+2")'

I did some testing with other base images and it seems like ruby:3.1-slim-bullseye with libjemalloc2 5.2.1-3 works fine but ruby:3.1-slim-bookworm with 5.3.0-1 does not. Installing the older jemalloc into Bookworm did not work so presumably it must be something else other than the jemalloc version?

@tisba
Copy link
Collaborator

tisba commented Nov 29, 2023

ah, jemalloc 😞

We had several Ruby and mini_racer issues with jemalloc in the past, e.g. #242.

I think that might be something @lloeki or @SamSaffron can take a look?

@tisba tisba added the bug label Nov 29, 2023
@ukolovda
Copy link
Author

I use jemalloc too...

@jasoncodes , thank you!

@lloeki
Copy link
Collaborator

lloeki commented Nov 29, 2023

I'm wondering whether it would get confused at times as to which malloc/free symbol is which depending on how the ruby jemalloc linking happens (I think glibc is always dynamic, dunno about musl).

https://github.com/jemalloc/jemalloc/wiki/Getting-Started

@lloeki
Copy link
Collaborator

lloeki commented Nov 29, 2023

Oh wait, I didn't see that:

ENV LD_PRELOAD=/usr/lib/libjemalloc.so.2

So you're injecting jemalloc, not linking Ruby against jemalloc at Ruby build time? Do you get the same result with a Ruby build against jemalloc?

@jasoncodes
Copy link

Do you get the same result with a Ruby build against jemalloc?

Good question. Looks to work fine if the Ruby is built using jemalloc instead of injecting.


I tested this using the first Ruby 3.2.2 w/ jemalloc base image I found which I could confirm has a working jemalloc with MALLOC_CONF=stats_print:true ruby -e exit:

FROM jiting/ruby:3.2.2-slim-bookworm
RUN apt-get update && apt-get -y install build-essential
RUN gem install mini_racer
CMD ruby -r mini_racer -e 'p MiniRacer::Context.new.eval("1+2")'

@lloeki
Copy link
Collaborator

lloeki commented Dec 1, 2023

Okay so I think what might happen is:

  • libv8 is built against glibc and links dynamically to it
  • This usually resolves the dynamic malloc+free symbols to glibc
  • When LD_PRELOAD is used to hook jemalloc in it applies to Ruby but it also applies to any other thing that dynamically links to libc's malloc+free (which would be any Ruby extension having linked to glibc)
  • We have mini_racer_loader that binds symbols with RTLD_LOCAL and RTLD_DEEPBIND when dlopening mini_racer_extension, this is supposed to make mini_racer_extension symbols hidden from outside and prefer its own internal symbols, but there is no malloc+free inside, so they resolve to glibc's malloc+free.
  • Even if there were malloc+free internal to mini_racer_extension , depending of how linking is done inside the mini_racer_extension shared library for these symbols (e.g malloc+free are internal but dynamic symbols) it might be so that LD_PRELOAD takes precedence over RTLD_DEEPBIND. Not the case here but we can entertain the thought.
  • Thus with LD_PRELOAD, libv8 will use jemalloc instead of glibc's malloc+free
  • libv8 does not like that, hilarity ensues

When ruby is built statically against jemalloc, the malloc+free symbols are entirely static (and might even be elided entirely, instead replaced by pure addresses), in which case they are unavailable to extensions. Thus Ruby can do its things with jemalloc and libv8 its things with glibc, and since ownership is split (nothing allocated from one is ever freed by the other) they both live happily ever after.

The remaining question is why libv8 would not like having its allocator swapped with jemalloc via LD_PRELOAD but I guess that's an upstream libv8 question.

@jasoncodes
Copy link

This morning I came across docker-library/ruby#182 (comment) which suggests using patchelf instead of LD_PRELOAD:

FROM ruby:3.2.2-slim-bookworm

RUN \
  apt-get update --yes && \
  DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
    build-essential libjemalloc2 patchelf && \
  patchelf --add-needed libjemalloc.so.2 /usr/local/bin/ruby

RUN gem install mini_racer

CMD ruby -r mini_racer -e 'p MiniRacer::Context.new.eval("1+2")'

This seems like a nicer way to patch an existing Ruby as it avoids affecting all processes. Unfortunately it doesn’t solve the problem here as dynamic linking still results in libv8 using jemalloc and crashing on exit. I’m guessing within the single Ruby process, patchelf and LD_PRELOAD are effectively the same thing.

Only option right now to use mini_racer with jemalloc seems to be building Ruby with jemalloc.

@tisba
Copy link
Collaborator

tisba commented Dec 13, 2023

Only option right now to use mini_racer with jemalloc seems to be building Ruby with jemalloc.

At least it seems to be more reliable. I'm running with LD_PRELOAD in production for a while and haven't had an issue so far; I'm still using ruby:3.2.2-bullseye (docker) though.

Since this is popping up again and again over time, I'm wondering if we should add something to the troubleshooting section of the README to recommend not using jmalloc for now, or use Ruby statically build against it. What do you think, @lloeki?

@jasoncodes
Copy link

FYI Rails has merged a PR to LD_PRELOAD jemalloc in their default Dockerfile template: rails/rails#50943.

In lieu of something in the README, may I suggest renaming this Issue to mention jemalloc?

@tisba tisba added bug/crash Bugs specific to crashes, segfaults, etc. Gem can be installed though. jemalloc The issue is related to the use of jemalloc labels May 6, 2024
SamSaffron added a commit that referenced this issue Aug 14, 2024
LD_PRELOAD can potentially cause issues with the "symbols hidden"
loader for mini_racer

This will unconditionally skip the loader when LD_PRELOAD is specified
and points to *malloc (eg jemalloc or tcmalloc)

see: #292 (comment)
@SamSaffron
Copy link
Collaborator

@lloeki maybe we simply try this?

#309

no loader if LD_PRELOAD is set to *malloc.

SamSaffron added a commit that referenced this issue Aug 14, 2024
LD_PRELOAD can potentially cause issues with the "symbols hidden"
loader for mini_racer

This will unconditionally skip the loader when LD_PRELOAD is specified
and points to *malloc (eg jemalloc or tcmalloc)

see: #292 (comment)

(also remove minitest/pride since is it no longer loading in CI)
@SamSaffron
Copy link
Collaborator

Note I have confirmation from @tgxworld that it resolved the issue for us, I went ahead and merged and pushed a release.

will follow up with a simply run of CI with jemalloc so this does not regress.

@nightpool
Copy link
Contributor

In lieu of something in the README, may I suggest renaming this Issue to mention jemalloc?

I don't really understand the loaders or dynamic linking enough to comment on the meat of this issue, but this suggestion seems sensible to me to help anyone trying to figure out what's crashing in the future ^

@SamSaffron SamSaffron changed the title Segmentation fault when application stops (Ruby 3.2.2, Segmentation fault when application stops (Ruby 3.2.2, tcmalloc or jemalloc) Aug 14, 2024
@SamSaffron
Copy link
Collaborator

that is easy enough @nightpool ... edited the title.

The fix feels fine, not too many memory allocators that people use in production don't have the word malloc in them.

@tisba
Copy link
Collaborator

tisba commented Aug 14, 2024

will follow up with a simply run of CI with jemalloc so this does not regress

Do you have a stable case to reproduce, @SamSaffron? I tried several times for a while, but either issues were Ruby related and have been fixed upstream, or I could not reliably reproduce it.

@SamSaffron
Copy link
Collaborator

I am not sure how easy it is to get a repro, a lot of these style of issues tend to require a rather hefty application behind it.

For example #300 is very hard to repro, only reliable repro I have is to spin up an entire Discourse application and run stuff on it. When I fish out all the eval mini racer makes, we no longer get a segfault.

I suspect this one is similar, we would probably need a reasonably heavy Ruby process with lots of allocations to and then to spin it down.

At a minimum though we can validate that require mini_racer_extension continues to work over time so people who ld preload do not get the symbol isolation (and can no longer run multiple versions of v8 under a single Ruby process) ... we can document this in the readme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/crash Bugs specific to crashes, segfaults, etc. Gem can be installed though. bug jemalloc The issue is related to the use of jemalloc
Projects
None yet
Development

No branches or pull requests

6 participants