rootwrap returns wrong exit code of subprocess

[Jakub Libosvar]

If child process of rootwrap is terminated by signal, exit code should be 128 + N where N is number of signal. According [1] subprocess.Popen.returncode returns -N. sys.exit interprets negative signed byte as unsigned thus original exit code is different.

e.g.
./rootwrap process # starts two processes
kill -9 $(pidof process) # Popen.returncode is -9, rootwrap calls sys.exit(-9)
echo $?
247 # instead of expected 137 (-9 signed char is binary 11110111 which is 247 in unsigned char)

 [1] https://docs.python.org/2/library/subprocess.html#subprocess.Popen.returncode

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/118569

[Yuriy Taraday (yorik-sar)]

Can you please point out why should return value be equal to 128 + signal number?

From what I see this value is used by shells when their children die because of signals, but I can't see any pointers if this is expected behavior for process wrappers.

[Jakub Libosvar (libosvar)]

http://www.tldp.org/LDP/abs/html/exitcodes.html

If you send SIGKILL to a child of sudo, sudo will exit with 137 (in BASH).

[Thierry Carrez (ttx)]

That sounds like the expected behavior in bash... but the convention (default) in subprocess.Popen is to return -N. Why should we prefer the former ? If there is no strong case for it (like an actual bug that would fix) I think I prefer to follow Python stdlib convention over bash convention...

[Jakub Libosvar (libosvar)]

I would agree with "I think I prefer to follow Python stdlib convention over bash convention" if rootwrap was meant to be a Python library. But it's rather a CLI tool and thus imo return code should be according bash convention.

[Doug Hellmann (doug-hellmann)]

Python's subprocess module converts the exit code to something easier for python programs calling subprocess to interpret: http://hg.python.org/cpython/file/7b0fdc1e917a/Lib/subprocess.py#l1343

We should probably have rootwrap exit with the same value as its child, since we want to treat it as a thin wrapper and transmit as much information as possible to the caller. When called from another python program via subprocess, that exit status will be re-converted to the negative signal value and when called from a shell script, the exit code could be interpreted using normal POSIX conventions of having the "exit code" in the low-order bits and the signal in the high-order bits.

Unfortunately, the actual exit status value is lost in the process because the raw value is never stored anywhere. I see two ways to fix that.

1. We could manually reverse the conversion, as in the proposed patch. I think this is probably safe, but it seems a little clunky.

2. We could subclass Popen() to replace the _handle_exitstatus() method with a version that saves the raw exit code and then calls the parent class implementation. The method is private to Popen, so this implementation is brittle.

I'm leaning toward keeping the implementation in the proposed patch (approach 1).

[Yuriy Taraday (yorik-sar)]

Now that I know that's POSIX(-ish) behavior, I think we should do that, too. But only for standalone rootwrap, daemon mode already passes raw Popen.return_status to caller. So we'll have a bit of a difference between how we handle standalone and daemon calls in the caller.

[Doug Hellmann (doug-hellmann)]

Isn't the return_status passed to the caller in daemon mode already passing through the -1*SIGNUM conversion? If so, I think the net effect will be the same for python programs using subprocess to invoke the command line tool and using the daemon mode.

[Yuriy Taraday (yorik-sar)]

I mean we'll need to pass return value from daemon as is and add some preprocessing to return value of standalone rootwrap call.

[Doug Hellmann (doug-hellmann)]

OK, I see what you mean. That sounds like the right thing to do.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/118569
Committed: https://git.openstack.org/cgit/openstack/oslo.rootwrap/commit/?id=56e9cb5b076ae2b5a687e50f940e459b506aa128
Submitter: Jenkins
Branch: master

commit 56e9cb5b076ae2b5a687e50f940e459b506aa128
Author: Jakub Libosvar <email address hidden>
Date: Wed Sep 3 10:34:57 2014 +0200

    Fix exit of subprocess in case it was terminated by signal

    If child process of rootwrap is terminated by signal, exit code should
    be 128 + N, where N is number of signal. According [1]
    subprocess.Popen.returncode returns -N. sys.exit interprets negative
    signed byte as unsigned thus original exit code is 256 - N instead of
    128 + N.

    [1]
    https://docs.python.org/2/library/subprocess.html#subprocess.Popen.returncode

    Change-Id: I43b4c959a98f8f620466c77de4cd5b724b09e5a8
    Closes-Bug: #1364822