lxd icon indicating copy to clipboard operation
lxd copied to clipboard

Better error handling in `lxc exec`

Open stgraber opened this issue 2 years ago • 3 comments

When a user attempts to execute a non-existing command or a file which isn't executable or anything else which causes an execve() error, the behavior on the lxc exec side is to not print anything on stdout or stderr and to just exit with the appropriate exit code.

stgraber@dakara:~$ lxc exec c1 non-existent
stgraber@dakara:~$ echo $?
127
stgraber@dakara:~$ lxc exec c1 /etc/passwd
stgraber@dakara:~$ echo $?
255
stgraber@dakara:~$ lxc exec v1 non-existent
stgraber@dakara:~$ echo $?
127
stgraber@dakara:~$ lxc exec v1 /etc/passwd
Error: fork/exec /etc/passwd: permission denied
stgraber@dakara:~$ echo $?
1
stgraber@dakara:~$ 

So first of all, we have a problem with the second v1 case here. We should make sure that lxd-agent behaves the same as liblxc and so returns no user-visible error and exits with 255.

Once we get the behavior properly lined up between containers and VMs, we should actually improve the user experience.

I believe we still want to stay away from putting anything on stdout or stderr through websocket as those should be reserved for actual output from the command. But I think it'd be fine to fail the operation and return an operation error instead.

This would then let lxc exec show that error to the user while being able to differentiate (at the API level) such an exec error from the executed command printing the same error and exitting with the same exit code.

stgraber avatar Jun 09 '22 19:06 stgraber

Sounds good!

tomponline avatar Jun 09 '22 19:06 tomponline

@stgraber can i assign this issue to myself and give it a try?

root-ali avatar Sep 04 '22 12:09 root-ali

Sure!

stgraber avatar Sep 04 '22 13:09 stgraber

@root-ali how are you getting on with this? Thanks

tomponline avatar Oct 10 '22 08:10 tomponline

We are in iran struggling with the fight with the regime and internet in our country under massive block from government and i cant test any of my code. I'm sorry about it but i searching for a way to test and create a pull request. @tomponline

root-ali avatar Oct 11 '22 07:10 root-ali

Ok thanks. Dont worry about this!

tomponline avatar Oct 11 '22 08:10 tomponline

@root-ali I can take this over if that is OK with you?

tomponline avatar Oct 11 '22 14:10 tomponline

I think we need to return 126 for both containers and VM when a file is found but is not executable.

https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html

If a command is not found, the child process created to execute it returns a status of 127. If a command is found but is not executable, the return status is 126.

tomponline avatar Oct 13 '22 09:10 tomponline

That looks like an lxc bug as lxc-attach has the same behaviour.

tomponline avatar Oct 13 '22 10:10 tomponline

@stgraber out of interest why does LXD have main_forkexec rather than using go-lxc's RunCommand*() functions? Is calling the liblxc's attach function not secure?

tomponline avatar Oct 13 '22 10:10 tomponline

The concern tends to be around a lot of the attach logic requiring running functions like setns which can affect entire threads or processes and could seriously confuse the Go runtime.

That's why we use forkXYZ for any liblxc interaction which would result in changes to namespaces.

stgraber avatar Oct 13 '22 13:10 stgraber