Add support for exiting on stdin or stdout EOF #159
Conversation
Various code that manipulates file descriptors assumes that they _are_ open. When changing this code, I don't want to consider the consequences of them not being open. Just check at startup that they are open and exit if they are not.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
==========================================
+ Coverage 77.75% 77.82% +0.06%
==========================================
Files 54 54
Lines 9498 9577 +79
==========================================
+ Hits 7385 7453 +68
- Misses 2113 2124 +11 ☔ View full report in Codecov by Sentry. |
|
Is there a way to mark some lines as “don’t expect to have coverage here”? The lines that don’t have coverage are things like “junk data protocol version” (but that is caught long before) or “program started with one of its standard streams closed” (but that means a bug in the caller). |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2024050800-4.2&flavor=pull-requests New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2024050210-4.2&flavor=update
Failed tests7 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/98585#dependencies 2 fixed
Unstable tests
|
I don't know, but sounds useful to have. Maybe gcov documentation says something about this? Or codecov one? |
There was a problem hiding this comment.
"Implement new version of process_io() that takes extensible struct" are partially included in "Add support for exiting on stdin or stdout EOF" - specifically the second iteration of process_io refactor. And also the commit message of the latter is outdated now - the global variables thing is not true.
agent/qrexec-agent-data.c
Outdated
|
|
||
| if (type == MSG_EXEC_CMDLINE) | ||
| if (type == MSG_EXEC_CMDLINE && pid > 0) | ||
| LOG(INFO, "pid %d exited with %d", pid, exit_code); |
There was a problem hiding this comment.
Can I get some log message on service exit?
There was a problem hiding this comment.
I think that should be a separate commit, since the current log message is completely unhelpful.
There was a problem hiding this comment.
It is helpful in that journald adds the logging process PID to the message, which can be easily correlated with the service start log.
| @@ -731,6 +742,18 @@ int find_qrexec_service( | |||
| path_buffer.data); | |||
| return -2; | |||
| } | |||
| if (cmd->exit_on_stdout_eof) { | |||
| LOG(ERROR, "Refusing to execute executable service %s with " | |||
| "exit-on-service-eof=true", | |||
There was a problem hiding this comment.
This is not the config name actually used.
| } | ||
| if (cmd->exit_on_stdin_eof) { | ||
| LOG(ERROR, "Refusing to execute executable service %s with " | ||
| "exit-on-client-eof=true", |
agent/qrexec-agent-data.c
Outdated
|
|
||
| if (type == MSG_EXEC_CMDLINE) | ||
| if (type == MSG_EXEC_CMDLINE && pid > 0) | ||
| LOG(INFO, "pid %d exited with %d", pid, exit_code); |
There was a problem hiding this comment.
I think that should be a separate commit, since the current log message is completely unhelpful.
That's a good question. Both require some documentation to clarify the behavior, but maybe the latter is a bit clearer? idk... |
I think so too. I kept using stdin/stdout internally, though, because that is what the file descriptors are named and so it makes the code easier to read. |
"pid 0 exited with 0" is not helpful. Just log "Socket service exited" and let the user use the PID recorded by systemd-journald to correlate that with the service start log.
This adds two new boolean service configuration options: - exit-on-service-eof: exit when the socket service shuts down its output stream for writing. - exit-on-client-eof: exit when the client shuts down its output stream for writing. The information is passed through an extended qrexec_parsed_command struct. New functions are added that avoid the executables having to access members that are private to libqrexec. The main use of these features is to emulate the old qubes.ConnectTCP and qubes.UpdatesProxy services, which already had this behavior due to the use of socat. These features are only supported for socket-based services, as executable services are more complicated and do not have a use case right now. Currently, if a service exits due to exit-on-stdin-eof, the empty MSG_DATA_STDOUT that indicates EOF is not sent. This is not a problem because qrexec-client-vm interprets MSG_DATA_EXIT_CODE as also indicating EOF on stdout and stderr. Fixes: QubesOS/qubes-issues#9176
The agent might exit before the empty MSG_DATA_STDIN message is sent.
It is always -1, and an assertion would fail if it was not.
If finding the service config fails with an I/O error, bail out promptly.
No functional change intended.
This adds two new boolean service configuration options:
To avoid compatibility problems, global variables are used to pass this information from the configuration parser to the I/O code. In the future, there should be a better way to pass this information, but this is not possible without more extensive changes. Fortunately, the configuration parser only runs once in the life of any process right now, so this commit adds assertions to check this. Qubes OS ships with assertions enabled, so any violation of this rule will be detected.
The main use of these features is to emulate the old qubes.ConnectTCP and qubes.UpdatesProxy services, which already had this behavior due to the use of socat.
These features are only supported for socket-based services, as executable services are more complicated and do not have a use case right now.
Currently, if a service exits due to exit-on-stdin-eof, the empty MSG_DATA_STDOUT that indicates EOF is not sent. This is not a problem because qrexec-client-vm interprets MSG_DATA_EXIT_CODE as also indicating EOF on stdout and stderr.
Fixes: QubesOS/qubes-issues#9176