BUG: define ActKillThread equal to ActKill #85
Conversation
These constants are equal in libseccomp but Go definitions were defined separately. This resulted in dead code that never executed due to identical case statements in switch. Go can usually detect these error cases and refuses to build but for some reason this detection doesn’t work with cgo+gcc. Clang detects the equal constants correctly and therefore libseccomp-golang builds with clang broke after ActKillThread was added. In order to fix the clang build only removal of the switch case is needed. But I assumed that the setter/getter logic is supposed to work for ActKillThread as well and only way to ensure that is to set them equal like they are in C. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
|
@kolyshkin ptal 🤗 |
pcmoore
changed the title
|
@pcmoore this LGTY? |
| // ActKillProcess kills the process that violated the rule. | ||
| // All threads in the thread group are also terminated. | ||
| // This action is only usable when libseccomp API level 3 or higher is | ||
| // supported. | ||
| ActKillProcess | ||
| // ActKillThread kills the thread that violated the rule. It is the same as ActKill. | ||
| // All other threads from the same thread group will continue to execute. | ||
| ActKillThread = ActKill |
There was a problem hiding this comment.
This may be very nitpicky, but if we are trying to be similar to the core libseccomp library it may be better to do:
ActKill = ActKillThread
... or would that still cause problems with clang?
There was a problem hiding this comment.
I think this is outside the scope of the bugfix. If libseccomp-golang wants to deprecate ActKill as well in favor or ActKillThread that SGTM but I think it is a separate decision. It would also mean reworking the current tests and some helper methods that currently use ActKill.
There was a problem hiding this comment.
I tend to think it is in scope, if we are changing how we define ActKillThread let's go ahead and do things properly so we are better in sync with the core library. A quick grep of the libseccomp-golang sources shows approximately two dozen places in the code that might be affected; that's not bad as far as I'm concerned.
I'm fine if you want to do this work in a separate patch, but please keep it in this PR.
|
To address #85 (comment), I opened #90 which swaps |
See seccomp/libseccomp-golang#85 for details. Also fixes seccomp/libseccomp-golang#89. Fixes: https://bugs.launchpad.net/bugs/1972119 Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
These constants are equal in libseccomp seccomp/libseccomp@b2f15f3#diff-26b243770fb00079467cda2a0d84f4d3d4158e565eac4570f12b08cbfe172be2R255 (with ActKillThread being a more precise name for the previous behavior) but Go definitions were defined separately in 24f2937.
This resulted in dead code that never executed due to identical case statements in switch. Go can usually detect these error cases and refuses to build but for some reason this detection doesn’t work with cgo+gcc.
Clang detects the equal constants correctly and therefore
libseccomp-golangbuilds with clang broke afterActKillThreadwas added making it impossible for us to build the latestrunc. https://gist.github.com/tonistiigi/ab6ef71781b7e3c5cabcb71a286a9243 is a simple example showing this condition where builds with clang fail.In order to fix the clang build only removal of the switch case is needed. But I assumed that the setter/getter
logic is supposed to work for
ActKillThreadas well and the only way to ensure that is to set them equal like theyare in C.
@kolyshkin @pjbgf
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com