Conversation
we shouldn't trust `waitKill` when we got the error `EINVAL`. Suggested-by: Sebastiaan van Stijn <github@gone.nl> Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: lifubang <lifubang@acmcoder.com>
| if waitKill, err := filter.GetWaitKill(); err != nil { | ||
| if !errors.Is(err, unix.EINVAL) { | ||
| return 0, false, fmt.Errorf("unable to fetch SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV bit: %w", err) | ||
| } | ||
| } else if waitKill { | ||
| flags |= uint(C.C_FILTER_FLAG_WAIT_KILLABLE_RECV) | ||
| } |
There was a problem hiding this comment.
If we really want to switch it over, I think better describes what we are doing, but I don't really mind too much either way.
| if waitKill, err := filter.GetWaitKill(); err != nil { | |
| if !errors.Is(err, unix.EINVAL) { | |
| return 0, false, fmt.Errorf("unable to fetch SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV bit: %w", err) | |
| } | |
| } else if waitKill { | |
| flags |= uint(C.C_FILTER_FLAG_WAIT_KILLABLE_RECV) | |
| } | |
| waitKill, err := filter.GetWaitKill() | |
| if errors.Is(err, unix.EINVAL) { | |
| waitKill, err = false, nil | |
| } | |
| if err != nil { | |
| return 0, false, fmt.Errorf("unable to fetch SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV bit: %w", err) | |
| } | |
| if waitKill { | |
| flags |= uint(C.C_FILTER_FLAG_WAIT_KILLABLE_RECV) | |
| } |
|
First of all I want to find out why we're getting EINVAL at all. See #5347 (comment) |
Proposed libseccomp-golang fix: seccomp/libseccomp-golang#127 With that, we won't have to add a special case for EINVAL |
rata
left a comment
rata
left a comment
There was a problem hiding this comment.
I think it's fine this or what @cyphar proposes. But the problem of returning EINVAL doesn't affect theoretically the other options too?
What about making this a helper function and calling it for each option we need. It could make filterFlags() simpler to read, and whenever we add a new filter flag, we will handle it correctly by using the helper.
What do you think?
Oh, I missed that. If the PR to handle that is merged, then it might be fine to just do this here. Sorry :-D |
|
My bad; still working on seccomp/libseccomp-golang#129; once it's in I will release a new libseccomp-golang version. |
|
Better late than never, https://github.com/seccomp/libseccomp-golang/releases/tag/v0.12.0 is out so we can just want for dependabot PR to bump it. Once it's there, we no longer need this PR not #5347 / #5354 I belive. |
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
So, GetWaitKill can return
- libseccomp-golang's errBadFilter ("filter is invalid or uninitialized")
- anything that seccomp_attr_get() can return (EACCES, EEXIST, EINVAL, EOPNOTSUPP)
- libseccomp-golang's
VersionError
I think this particular code should just ignore ALL errors, and only set the attribute if true, nil is returned.
OTOH, with libseccomp-golang v0.12, if libseccomp.GetAPI returns a level 7 or greater, it means that filter.GetWaitKill works and should not return an error. Meaning, the current code (after commit 177f385) is just fine.
|
closing this as the workaround was removed and any error is a real error. |
4 participants
we shouldn't trust
waitKillwhen we got the errorEINVAL.Ref:
#5367 (comment)
#5367 (comment)