Skip to content

Fix forwarded process signal encoding#1778

Open
vincent-peng wants to merge 3 commits into
apple:mainfrom
vincent-peng:fix-forwarded-process-signals
Open

Fix forwarded process signal encoding#1778
vincent-peng wants to merge 3 commits into
apple:mainfrom
vincent-peng:fix-forwarded-process-signals

Conversation

@vincent-peng

Copy link
Copy Markdown

Summary

  • Encode forwarded process signals as signal names instead of integer XPC values.
  • Preserve Darwin-to-Linux signal semantics for foreground signal forwarding.
  • Add coverage for the kill request payload and forwarded INT, TERM, WINCH, USR1, and USR2 mappings.

Root cause

Foreground non-tty signal forwarding sent .signal as an integer, but the API server reads that field as a string before forwarding it to the runtime. That made forwarded resize signals fail with missing signal in xpc message. A numeric-only fix would also risk mis-mapping Darwin USR1/USR2 values to Linux.

Fixes #1747

Validation

  • swift test --filter ContainerAPIClientTests.ClientProcessTests
  • swift test --filter ContainerAPIClientTests
  • swift build --target ContainerAPIClient

@JeronimoColon

Copy link
Copy Markdown

I hit this independently while building on ContainerAPIClient, in a form broader than the resize noise in #1747: ClientProcess.kill fails for every signal, so there is no working way to signal an exec'd process at all. A timeout built on createProcess can stop waiting for a command, but cannot terminate it. I filed #1941 with a standalone reproducer and the root-cause trace.

Two notes from checking this change against the server path:

  • The encoding checks out. The server-side parse (Signal.init(_:) in containerization) uppercases, trims an optional SIG prefix, and resolves the name against the Linux table, so the prefix-less names this PR sends parse correctly. That parser and its tables are unchanged from containerization 0.33.3 through current main, so a client carrying this fix also works against already-installed 1.0.0 and 1.1.0 API servers.
  • Consider adding a KILL case to the tests. Timeout and cancel implementations mostly send SIGKILL, so it is the value most callers will hit first. It passes the guard today (Signal.platformName(SIGKILL) resolves to "KILL", which is in the Linux table); a test would pin that.

Sending names instead of numbers also avoids the Darwin/Linux mismatch on USR1 and USR2, which a numeric fix would have gotten wrong. Nice work.

@vincent-peng

Copy link
Copy Markdown
Author

Thanks, this was helpful. Added explicit SIGKILL coverage in b08d725 to pin the timeout/cancel path.

I also added a Darwin-only SIGINFO rejection case so the client-side guard keeps host-only signal names from reaching the runtime. Keeping the name-based encoding unchanged so USR1/USR2 continue mapping by signal name rather than Darwin numeric value.

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.

[Bug]: Terminal resize SIGWINCH forwarding prints missing signal in xpc message

2 participants