gh-96652: Fix faulthandler chained signal without sigaction() #96666

vstinner · 2022-09-07T20:51:29Z

Fix the faulthandler implementation of faulthandler.register(signal, chain=True) if the sigaction() function is not available: don't call the previous signal handler if it's NULL.

Issue: Possible NULL-pointer dereference in faulthandler_user #96652

Fix the faulthandler implementation of faulthandler.register(signal, chain=True) if the sigaction() function is not available: don't call the previous signal handler if it's NULL.

sobolevn · 2022-09-08T08:02:21Z

Related: #96660

vstinner · 2022-09-08T09:30:11Z

@sobolevn @ovchinnikov-v-a: Could you test/confirm that this change fix #96652?

sobolevn · 2022-09-08T09:44:19Z

I've compiled your PR and got this:

Python 3.12.0a0 (heads/faulthandler:65b693f1c3, Sep  8 2022, 12:42:23) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import faulthandler
>>> import signal
>>> import time
>>> 
>>> faulthandler.register(signal.SIGALRM, chain=True)
>>> signal.alarm(1)
0
>>> time.sleep(2)
print("exit")
Current thread 0x000000010feec5c0 (most recent call first):
  File "<stdin>", line 1 in <module>
[1]    57506 alarm      ./python.exe

I think we need a test case!

ovchinnikov-v-a · 2022-09-08T09:46:03Z

@sobolevn @ovchinnikov-v-a: Could you test/confirm that this change fix #96652?

@vstinner test_socketserver with amendments from #96652 passes with this PR applied:

0:00:00 load avg: 1.39 Run tests in parallel using 1 child processes
0:01:00 load avg: 0.78 running: test_socketserver (1 min)
0:01:30 load avg: 0.61 running: test_socketserver (1 min 30 sec)
...
0:17:05 load avg: 1.05 [1/1] test_socketserver passed (17 min 4 sec)
test_forking_handled (test.test_socketserver.ErrorHandlerTest.test_forking_handled) ... ok
test_forking_not_handled (test.test_socketserver.ErrorHandlerTest.test_forking_not_handled) ... ok
...
test_shutdown (test.test_socketserver.SocketServerTest.test_shutdown) ... ok
test_tcpserver_bind_leak (test.test_socketserver.SocketServerTest.test_tcpserver_bind_leak) ... Current thread 0x00007f803ac4c740 (most recent call first):
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/test_socketserver.py", line 294 in test_tcpserver_bind_leak
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/case.py", line 579 in _callTestMethod
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/case.py", line 623 in run
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/case.py", line 678 in __call__
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/suite.py", line 122 in run
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/suite.py", line 122 in run
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/suite.py", line 122 in run
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/unittest/runner.py", line 208 in run
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/support/__init__.py", line 1100 in _run_suite
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/support/__init__.py", line 1226 in run_unittest
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/libregrtest/runtest.py", line 281 in _test_module
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/libregrtest/runtest.py", line 317 in _runtest_inner2
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/libregrtest/runtest.py", line 360 in _runtest_inner
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/libregrtest/runtest.py", line 235 in _runtest
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/libregrtest/runtest.py", line 265 in runtest
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/libregrtest/runtest_mp.py", line 95 in run_tests_worker
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/libregrtest/main.py", line 722 in _main
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/libregrtest/main.py", line 701 in main
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/libregrtest/main.py", line 763 in main
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/regrtest.py", line 43 in _main
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/test/regrtest.py", line 47 in <module>
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/runpy.py", line 88 in _run_code
  File "/home/vitalii/Projects/CPython/ovchinnikov-v-a-cpython/Lib/runpy.py", line 198 in _run_module_as_main
ok
test_basics (test.test_socketserver.SocketWriterTest.test_basics) ... ok
test_write (test.test_socketserver.SocketWriterTest.test_write) ... ok

----------------------------------------------------------------------
Ran 27 tests in 1024.868s

OK

== Tests result: SUCCESS ==

1 test OK.

Total duration: 17 min 5 sec
Tests result: SUCCESS

ovchinnikov-v-a · 2022-09-08T09:52:23Z

I've compiled your PR and got this:

Python 3.12.0a0 (heads/faulthandler:65b693f1c3, Sep  8 2022, 12:42:23) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import faulthandler
>>> import signal
>>> import time
>>> 
>>> faulthandler.register(signal.SIGALRM, chain=True)
>>> signal.alarm(1)
0
>>> time.sleep(2)
print("exit")
Current thread 0x000000010feec5c0 (most recent call first):
  File "<stdin>", line 1 in <module>
[1]    57506 alarm      ./python.exe

I think we need a test case!

Looks fine: you've got traceback printed by faulthandler_user->faulthandler_dump_traceback and no segmentation fault.

miss-islington · 2022-09-08T10:20:26Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

bedevere-bot · 2022-09-08T10:20:38Z

GH-96679 is a backport of this pull request to the 3.11 branch.

bedevere-bot · 2022-09-08T10:20:43Z

GH-96680 is a backport of this pull request to the 3.10 branch.

…ythonGH-96666) Fix the faulthandler implementation of faulthandler.register(signal, chain=True) if the sigaction() function is not available: don't call the previous signal handler if it's NULL. (cherry picked from commit c580a81) Co-authored-by: Victor Stinner <[email protected]>

vstinner · 2022-09-08T10:22:52Z

no segmentation fault.

Good :-) I merged my PR.

I think we need a test case!

test_register_chain() tests chain=True, but the test sets a signal handler for SIGUSR1. It doesn't test the case with a NULL signal handler. I suppose that NULL means "default signal handler". The problem is that I don't think that operating systems have a portable behavior for the default signal handler. Is the signal ignored? Is the process killed? I would suggest to not add a test relying on the default signal handler.

Fix the faulthandler implementation of faulthandler.register(signal, chain=True) if the sigaction() function is not available: don't call the previous signal handler if it's NULL. (cherry picked from commit c580a81) Co-authored-by: Victor Stinner <[email protected]>

ovchinnikov-v-a · 2022-09-08T10:45:33Z

I suppose that NULL means "default signal handler". The problem is that I don't think that operating systems have a portable behavior for the default signal handler. Is the signal ignored? Is the process killed?

Yep, SIG_DFL appears to be defined as a NULL pointer, but there is no POSIX-defined behavior for our case:

void (*signal(int sig, void (*func)(int)))(int);
...
When a signal occurs, and `func` points to a function, it is implementation-defined whether the equivalent of a `signal(sig, SIG_DFL);` is executed or the implementation prevents some implementation-defined set of signals (at least including `sig`) from occurring until the current signal handling has completed.

Fix the faulthandler implementation of faulthandler.register(signal, chain=True) if the sigaction() function is not available: don't call the previous signal handler if it's NULL. (cherry picked from commit c580a81) Co-authored-by: Victor Stinner <[email protected]>

gh-96652: Fix faulthandler chained signal without sigaction()

65b693f

Fix the faulthandler implementation of faulthandler.register(signal, chain=True) if the sigaction() function is not available: don't call the previous signal handler if it's NULL.

vstinner added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Sep 7, 2022

bedevere-bot added the awaiting core review label Sep 7, 2022

vstinner mentioned this pull request Sep 7, 2022

Possible NULL-pointer dereference in faulthandler_user #96652

Closed

ovchinnikov-v-a mentioned this pull request Sep 8, 2022

gh-96652: Check previous signal handler to be non-NULL #96660

Closed

vstinner merged commit c580a81 into python:main Sep 8, 2022

vstinner deleted the faulthandler branch September 8, 2022 10:20

bedevere-bot removed awaiting core review needs backport to 3.11 only security fixes labels Sep 8, 2022

bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-96652: Fix faulthandler chained signal without sigaction() #96666

gh-96652: Fix faulthandler chained signal without sigaction() #96666

vstinner commented Sep 7, 2022 •

edited by bedevere-bot

Loading

sobolevn commented Sep 8, 2022

vstinner commented Sep 8, 2022

sobolevn commented Sep 8, 2022

ovchinnikov-v-a commented Sep 8, 2022 •

edited

Loading

ovchinnikov-v-a commented Sep 8, 2022

miss-islington commented Sep 8, 2022

bedevere-bot commented Sep 8, 2022

bedevere-bot commented Sep 8, 2022

vstinner commented Sep 8, 2022

ovchinnikov-v-a commented Sep 8, 2022

gh-96652: Fix faulthandler chained signal without sigaction() #96666

gh-96652: Fix faulthandler chained signal without sigaction() #96666

Conversation

vstinner commented Sep 7, 2022 • edited by bedevere-bot Loading

sobolevn commented Sep 8, 2022

vstinner commented Sep 8, 2022

sobolevn commented Sep 8, 2022

ovchinnikov-v-a commented Sep 8, 2022 • edited Loading

ovchinnikov-v-a commented Sep 8, 2022

miss-islington commented Sep 8, 2022

bedevere-bot commented Sep 8, 2022

bedevere-bot commented Sep 8, 2022

vstinner commented Sep 8, 2022

ovchinnikov-v-a commented Sep 8, 2022

vstinner commented Sep 7, 2022 •

edited by bedevere-bot

Loading

ovchinnikov-v-a commented Sep 8, 2022 •

edited

Loading