Skip to content
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

runc-dmz masks the error from unix.SYS_EXECVEAT #4170

Closed
lifubang opened this issue Jan 11, 2024 · 12 comments
Closed

runc-dmz masks the error from unix.SYS_EXECVEAT #4170

lifubang opened this issue Jan 11, 2024 · 12 comments

Comments

@lifubang
Copy link
Member

Build runc from the main branch in a amd64 host, and use a different architecture container entrypoint to start the container, we will get no error msg, it's different from 1.1.

root@iZj6cdnzj9sp96xf38htrnZ:~/ubuntutest# /root/go/src/github.com/opencontainers/runc/runc run ctest
root@iZj6cdnzj9sp96xf38htrnZ:~/ubuntutest# echo $?
255
root@iZj6cdnzj9sp96xf38htrnZ:~/ubuntutest# runc -v
runc version 1.1.7-0ubuntu1~20.04.1
spec: 1.0.2-dev
go: go1.18.1
libseccomp: 2.5.1
root@iZj6cdnzj9sp96xf38htrnZ:~/ubuntutest# runc run ctest
exec /runc.arm64: exec format error
root@iZj6cdnzj9sp96xf38htrnZ:~/ubuntutest# echo $?
1

There are two problems:

  1. The exit status code 255 is right in the main branch, but 1 in 1.1.* is wrong.
  2. The error msg exec /runc.arm64: exec format error is gone in the main branch.
@lifubang
Copy link
Member Author

lifubang commented Jan 12, 2024

Because runc-dmz always has the same architecture with runc, so unix.SYS_EXECVEAT will always be success. But in runc-dmz, when we call execve, we may get an error, so we should translate the errno to errmsg in runc-dmz, but there is no strerror in nolibc. WDYT @rata , could you give us some suggestion?
How about display the error like this:

exec ***** with an error(errno=8)

But people may not know what's the 8 mean.

@rata
Copy link
Member

rata commented Jan 12, 2024

We can use perror(), but the nolibc version doesn't expand the message there either. It will be shown like this:

$ sudo ../runc/runc run mycontainer
execve: errno=8
$ echo $?
255

This is with this patch:

$ sudo ../runc/runc run mycontainer
execve: errno=8
rodrigo@lindsay: ~/src/github.com/opencontainers/mycontainer$ echo $?
255

However, I don't think we have anything better to do if we use nolibc. Even if we create a translation (it is easy to do so using the errno command), it won't be locale aware for the user.

errno is arch dependant, we can't do that easily. I think we can just fix the return code and we have to live with this if we keep nolibc :-/

@rata
Copy link
Member

rata commented Jan 12, 2024

@lifubang I think this another thing to add to the list of downside of #4158 :(

@rata
Copy link
Member

rata commented Jan 12, 2024

Another option might be to return the errno in _dmz.c. Then, the go program takes that and translates it using the go library (https://pkg.go.dev/golang.org/x/sys/unix#ErrnoName).

I'll check this out.

@cyphar
Copy link
Member

cyphar commented Jan 12, 2024

If this cannot be solved simply, we are deleting runc-dmz as there appear to be very deep issues that we will miss no longer how many times we find a "simple" surface bug.

I liked the idea, but Linux doesn't allow you to do that in general it seems.

@lifubang
Copy link
Member Author

I think this another thing to add to the list of downside of #4158 :(

I don't think this problem is so serious enough that we need to do this.
I'll give some fix ways to discuss.

@lifubang
Copy link
Member Author

lifubang commented Jan 14, 2024

  1. It's easy to display the err msg like this: exec *******: errno=*, but users may not know what's the meaning;
  2. Hard code the error msg list:
    2.1 Generate the error msg constants when compiling runc, we can use the way like this: https://cs.opensource.google/go/x/sys/+/master:unix/mkerrors.sh
    The disadvantage of this method is that most of the generated error msg constants are not needed by dmz.
    2.2 Hard code the necessary error msgs in _dmz.c, because according to the doc of execve, there are no more than 20 possible errors that could be returned by execve.
    Please see: https://man7.org/linux/man-pages/man2/execve.2.html

@lifubang
Copy link
Member Author

I prefer to 2.2, welcome different opinions.

@rata
Copy link
Member

rata commented Jan 15, 2024

@lifubang I think I prefer the PR I opened (#4172 ).

It is trivial, it prints the same message as before, and even using libc the size difference with the runc binary is more than 21x (runc is 14100K, runc-dmz with libc is 668K). IMHO we can look into those alternatives for runc 1.3, if we still want to keep runc-dmz AND reduce its size. I don't see the need to invest more in runc-dmz now.

@lifubang
Copy link
Member Author

Yes, yes, it’s obvious an option. @cyphar @AkihiroSuda PTAL

@lifubang
Copy link
Member Author

@lifubang I think this another thing to add to the list of downside of #4158 :(

Added to #4114

@lifubang
Copy link
Member Author

This is resolved by #4172, I will open an new issue to track the problem of the exit status code error in release-1.1 branch and main branch without dmz.

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

No branches or pull requests

3 participants