-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: AddRule: fix to handle EACCES #74
Conversation
seccomp_test.go
Outdated
if err == nil { | ||
t.Error("expected error, got nil") | ||
} else { | ||
const exp = "matches default action" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can introduce
var errRuleActionMatchesDefault = errors.New("requested action matches default action of filter")
in the code, and compare against it here in the test. This way, if the wording is ever changed, the test would not need fixing (but will still check the error returned is as expected).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented.
The latter is to be used by the next commit. While at it, slightly improve the errBadFilter doc string. NOTE that there are more places that need s/fmt.Errorf/errors.New/, but as such change is trivial and it will break other commit pending review, I am going to implement it separately later. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case a rule with the action that equals to the default one was added, libseccomp used to return EPERM, and libseccomp-golang converted it into a more user-friendly "requested action matches default action of filter" error. From various bug reports I have noticed this is no longer a case. The cause is libseccomp commit 83989be02 (appeared in v2.5.0), which changes EPERM to EACCES. Since we still support libseccomp < 2.5.0, check for either EPERM or EACCES. Add a TODO item to remove the former. Add a test case, which fails like this before the fix: > seccomp_test.go:580: expected error to contain "matches default action", got "permission denied" Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Hmmm, I think @drakenclimber and I may need to sort an aspect of this out in the main libseccomp library first as the Either we need to update the docs (the manpage) or we need to shift to using one of the approved error codes, |
As I look at this I continue to be a bit torn, |
Interesting. For good or for bad, libseccomp proper is consistent with error codes ( Paul is correct in that we need to fix something (code and/or manpages) in libseccomp. Late on a Friday of a long week, I would vote for returning |
... yet the commit seccomp/libseccomp@83989be02 I refer to in description says "[t]his is part of our error code cleanup and API promise".
My take on that is, since either EACCES or EPERM can be misinterpreted, let's stick with the one we already have (since 2.5.0), i.e. EACCES. Changing it back to EPERM, it seems, will do more harm than good.
I agree. Filed seccomp/libseccomp#339 so we can continue discussion in there. Moving this PR to draft for now. |
As a quick FYI, seccomp/libseccomp#339 has now been merged upstream. |
No longer a draft. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thanks, @kolyshkin.
One question regarding your comment in the first commit about switching from fmt.Errorf
to errors.New
. Should we someday consider going even further and defining a seccomp Error Type (or perhaps multiple Error Types) and propagate that to the user? Then the user could get out of the business of parsing seccomp error strings. Thoughts?
Acked-by: Tom Hromatka <tom.hromatka@oracle.com>
The same outcome can be achieved easily by merely exporting the errors that are now internal. Say, if we have var (
ErrBadFilter = errors.New("filter is invalid or uninitialized")
ErrDefAction = errors.New("requested action matches default action of filter")
) a user can merely compare the returned error against these values either directly (no longer recommended since Go 1.13 error wrapping was introduced) or using if errors.Is(err, libseccomp.ErrDefAction) {
... For more complicated cases where error message is not fixed (i.e. contains some more context) there are also ways to make it works with That's a separate issue though (and I will look into it later). |
@pcmoore PTAL |
A quick note: please refrain from using "PTAL", I personally find it very annoying. I get notified of all the activity in the repo and I'm well aware there have been updates; if I haven't responded it is because I haven't had the time. With that out of the way, it has been a busy week with other things, I'll try to get back to this early next week. |
Thanks for your patience, I just merged this at HEAD 77bddc2. |
In case a rule with the action that equals to the default one was added,
libseccomp used to return EPERM, and libseccomp-golang converted it into
a more user-friendly "requested action matches default action of
filter" error.
From various bug reports I have noticed this is no longer a case.
The cause is seccomp/libseccomp@83989be02 (appeared in v2.5.0), which
changes EPERM to EACCES.
Since we still support libseccomp < 2.5.0, check for either EPERM or
EACCES, and add a TODO item to remove the former.
Add a test case, which fails like this before the fix: