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

Preserve existing SIGPROF signal handler on passenger start #2496

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

ivoanjo
Copy link
Contributor

@ivoanjo ivoanjo commented Aug 31, 2023

What does this PR do?

This PR tweaks the list of trappable signals (Signal.list_trappable in ruby_core_enhancements.rb) to consider SIGPROF a non-trappable signal.

This fixes the issue reported in #2489 where a profiler (or other application) installed a SIGPROF handler, and then the handler was reset and thus the application would crash upon receiving it.

As requested in this comment I've also gone ahead and filled/submitted the contributor agreement form.

How to test the change?

The following config.ru reproducer is able to show the problem:

puts "Installing SIGPROF signal handler!"

Signal.trap("PROF") do
  puts " !! Got sigprof!"
end

puts "Testing signal handler!"

Process.kill("PROF", Process.pid)

puts "Running rack app!"

app = ->(env) {
  puts " ** Got request!"

  if env['REQUEST_URI'].start_with?('/signal')
    puts "Sending signal to myself!"
    Process.kill("PROF", Process.pid)
  end

  [200, {}, ['Hello, World!']]
}
run app

Before this change:

$ bundle exec passenger start
=============== Phusion Passenger(R) Standalone web server started ===============
PID file: passenger.3000.pid
Log file: passenger.3000.log
Environment: development
Accessible via: http://0.0.0.0:3000/

You can stop Phusion Passenger(R) Standalone by pressing Ctrl-C.
Problems? Check https://www.phusionpassenger.com/library/admin/standalone/troubleshooting/
===============================================================================
[ N 2023-07-25 10:13:12.5640 204259/T5 age/Cor/SecurityUpdateChecker.h:519 ]: Security update check: no update found (next check in 24 hours)
App 204333 output: Installing SIGPROF signal handler!
App 204333 output: Testing signal handler!
App 204333 output:  !! Got sigprof!
App 204333 output: Running rack app!
App 204368 output:  ** Got request! # <-- Regular request
App 204368 output:  ** Got request! # <-- I hit /signal
App 204368 output: Sending signal to myself!
[ W 2023-07-25 10:13:27.9117 204259/Te age/Cor/Con/InternalUtils.cpp:96 ]: [Client 4-1] Sending 502 response: application did not send a complete response
[ W 2023-07-25 10:13:30.0219 204259/T3 age/Cor/App/Poo/AnalyticsCollection.cpp:101 ]: Process (pid=204368, group=example (development)) no longer exists! Detaching it from the pool.
[ N 2023-07-25 10:13:30.0220 204259/T3 age/Cor/CoreMain.cpp:1146 ]: Checking whether to disconnect long-running connections for process 204368, application example (development)

(Every request fails because the worker crashes, and then gets replaced with another one that crashes when it gets the next request, and so on)

After this change:

$ bundle exec passenger start
=============== Phusion Passenger(R) Standalone web server started ===============
PID file: passenger.3000.pid
Log file: passenger.3000.log
Environment: development
Accessible via: http://0.0.0.0:3000/

You can stop Phusion Passenger(R) Standalone by pressing Ctrl-C.
Problems? Check https://www.phusionpassenger.com/library/admin/standalone/troubleshooting/
===============================================================================
[ N 2023-08-25 14:39:17.6888 540283/T5 age/Cor/SecurityUpdateChecker.h:519 ]: Security update check: no update found (next check in 24 hours)
App 540356 output: Installing SIGPROF signal handler!
App 540356 output: Testing signal handler!
App 540356 output:  !! Got sigprof!
App 540356 output: Running rack app!
App 540391 output:  ** Got request!
App 540391 output:  ** Got request!
App 540391 output: Sending signal to myself! # <-- No issues in request!
App 540391 output:  !! Got sigprof!
App 540391 output:  ** Got request!
App 540391 output: Sending signal to myself! ! <-- Works! :)
App 540391 output:  !! Got sigprof!

I've also checked with the dd-trace-rb, it no longer needs a workaround when run under passenger (this workaround reduces the accuracy of profiling data, hence my interest in not having it).

Once this fix gets released, I plan to add newer versions of passenger to the allow-list, so dd-trace-rb will not apply the workaround for them.

Fixes #2489

**What does this PR do?**

This PR tweaks the list of trappable signals (`Signal.list_trappable`
in `ruby_core_enhancements.rb`) to consider `SIGPROF` a non-trappable
signal.

This fixes the issue reported in phusion#2489 where a profiler (or other
application) installed a `SIGPROF` handler, and then the handler was
reset and thus the application would crash upon receiving it.

**How to test the change?**

The following `config.ru` reproducer is able to show the problem:

```
puts "Installing SIGPROF signal handler!"

Signal.trap("PROF") do
  puts " !! Got sigprof!"
end

puts "Testing signal handler!"

Process.kill("PROF", Process.pid)

puts "Running rack app!"

app = ->(env) {
  puts " ** Got request!"

  if env['REQUEST_URI'].start_with?('/signal')
    puts "Sending signal to myself!"
    Process.kill("PROF", Process.pid)
  end

  [200, {}, ['Hello, World!']]
}
run app
```

Before this change:

```
$ bundle exec passenger start
=============== Phusion Passenger(R) Standalone web server started ===============
PID file: passenger.3000.pid
Log file: passenger.3000.log
Environment: development
Accessible via: http://0.0.0.0:3000/

You can stop Phusion Passenger(R) Standalone by pressing Ctrl-C.
Problems? Check https://www.phusionpassenger.com/library/admin/standalone/troubleshooting/
===============================================================================
[ N 2023-07-25 10:13:12.5640 204259/T5 age/Cor/SecurityUpdateChecker.h:519 ]: Security update check: no update found (next check in 24 hours)
App 204333 output: Installing SIGPROF signal handler!
App 204333 output: Testing signal handler!
App 204333 output:  !! Got sigprof!
App 204333 output: Running rack app!
App 204368 output:  ** Got request! # <-- Regular request
App 204368 output:  ** Got request! # <-- I hit /signal
App 204368 output: Sending signal to myself!
[ W 2023-07-25 10:13:27.9117 204259/Te age/Cor/Con/InternalUtils.cpp:96 ]: [Client 4-1] Sending 502 response: application did not send a complete response
[ W 2023-07-25 10:13:30.0219 204259/T3 age/Cor/App/Poo/AnalyticsCollection.cpp:101 ]: Process (pid=204368, group=example (development)) no longer exists! Detaching it from the pool.
[ N 2023-07-25 10:13:30.0220 204259/T3 age/Cor/CoreMain.cpp:1146 ]: Checking whether to disconnect long-running connections for process 204368, application example (development)
```

(Every request fails because the worker crashes, and then gets replaced
with another one that crashes when it gets the next request, and so on)

After this change:

```
$ bundle exec passenger start
=============== Phusion Passenger(R) Standalone web server started ===============
PID file: passenger.3000.pid
Log file: passenger.3000.log
Environment: development
Accessible via: http://0.0.0.0:3000/

You can stop Phusion Passenger(R) Standalone by pressing Ctrl-C.
Problems? Check https://www.phusionpassenger.com/library/admin/standalone/troubleshooting/
===============================================================================
[ N 2023-08-25 14:39:17.6888 540283/T5 age/Cor/SecurityUpdateChecker.h:519 ]: Security update check: no update found (next check in 24 hours)
App 540356 output: Installing SIGPROF signal handler!
App 540356 output: Testing signal handler!
App 540356 output:  !! Got sigprof!
App 540356 output: Running rack app!
App 540391 output:  ** Got request!
App 540391 output:  ** Got request!
App 540391 output: Sending signal to myself! # <-- No issues in request!
App 540391 output:  !! Got sigprof!
App 540391 output:  ** Got request!
App 540391 output: Sending signal to myself! ! <-- Works! :)
App 540391 output:  !! Got sigprof!
```

I've also checked with the dd-trace-rb, it no longer needs a workaround
when run under passenger (this workaround reduces the accuracy of
profiling data, hence my interest in not having it).

Once this fix gets released, I plan to add newer versions of passenger
to the allow-list, so dd-trace-rb will not apply the workaround for
them.

Fixes phusion#2489
@ivoanjo
Copy link
Contributor Author

ivoanjo commented Sep 11, 2023

Hey @CamJN would this be reasonable to merge in?

I'm pinging you since you replied to #2489 -- I'd love to have this in a stable release of passenger :)

@CamJN
Copy link
Member

CamJN commented Sep 11, 2023

Yeah it looks good, I'm just a wee bit busy so I hadn't looked at it until now.

@CamJN CamJN merged commit e7d2412 into phusion:stable-6.0 Sep 11, 2023
@ivoanjo
Copy link
Contributor Author

ivoanjo commented Sep 11, 2023

Thanks a lot!

@ivoanjo ivoanjo deleted the ivoanjo/preserve-existing-sigprof branch September 12, 2023 08:02
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 this pull request may close these issues.

Passenger resets SIGPROF signal handlers causing profilers to crash application
2 participants