Avoid deadlocks after fork#303
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
bcb913a to
2a70190
Compare
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
2a70190 to
9cc08d6
Compare
9cc08d6 to
67c0d0f
Compare
67c0d0f to
9c28979
Compare
e249b48 to
4121230
Compare
| }; | ||
|
|
||
| const auto result = ::setrlimit(resource, &limit); | ||
| SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE( |
There was a problem hiding this comment.
I think we have the same async signal safe issue with the assert message, as the string concatenation may cause heap allocation which is not async signal safe
There was a problem hiding this comment.
We could remove the extra info and just use strerror as the message
There was a problem hiding this comment.
Update: strerror is not async signal safe either, there is strerrordesc_np but it's not standard
| }; | ||
|
|
||
| const auto result = ::setrlimit(resource, &limit); | ||
| SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE( |
There was a problem hiding this comment.
bazel assort macros terminate the process using std::abort(). Is this safe to terminate the process this way btw. fork() and execve()?
There was a problem hiding this comment.
abort is async signal safe in POSIX, not sure if std::abort is doing anything extra?
There was a problem hiding this comment.
I think here it is not so much about the signal safety, but other considerations what is being cleaned up: https://www.unixguide.net/unix/programming/1.1.3.shtml
Not sure though exactly what is the difference btw. _exit and std::abort, but If I remember correctly using _exit() is the recommended way to terminate a process btw. fork and execve in case of error
There was a problem hiding this comment.
Some more info- std::abort and abort are one and the same. abort does no more cleanup than _exit. The main difference is that abort sends SIGABRT which triggers a core dump whereas _exit ends the process normally
|
One thing we need to consider: the assert handler implementation as is looks quite okay for all non AS safe use cases. So we might want write an own function which does AS safe prints with write(STDERR_FILENO, msg, msg_len), msg is a fixed string to avoid allocations (e.g long string + std::string("something")). Our own function does sysexit() after the print. |
This ensures that security properties are not accidentally left unset.
4121230 to
6adb4b7
Compare
The executable could be incorrectly seen as missing if it is on a filesystem which has not been mounted yet. `execve` already fails if the executable does not exist so there is no need to check in advance.
4 participants
Fork only copies the current thread, so from that point on we could deadlock if we access anything that was locked by another thread at the time of the fork.
As such we should play it safe and use only basic functions. This means assertions rather than logs for fatal errors, and no logging at all on the happy path.