status_of_proc() calls pidofproc() which calls kill, requiring ownership privileges on the process

Bug #246735 reported by Dustin Kirkland 
10
Affects Status Importance Assigned to Milestone
lsb (Ubuntu)
Medium
Dustin Kirkland 

Bug Description

Binary package hint: lsb

The status_of_proc() function is intended to provide a reusable framework for individual init scripts to provide a "status" action, whereby users and query the init script and determine if a given service is running.

The status_of_proc() function calls the pidofproc() function. This function will first attempt to locate a pidfile, and if found, will call "kill -0" to send a signal to the process. The operation requires that the euid of the user querying the status of the service either match that of the service, or of course be root.

Non-root, non-owning users should be able to query the status of services. For this reason, the "kill -0" test is not going to work.

Further down in the pidofproc() function, it falls back upon calling /bin/pidof. This is actually the functionality that we need in the status_of_proc() function.

The attached patch adjusts the body of the status_of_proc() function to use /bin/pidof such that any user can query the status of system services.

I will push this patch to Debian and link the bug number to this report.

:-Dustin

Related branches

Revision history for this message
Dustin Kirkland  (kirkland) wrote :

Patch. Requesting sponsorship.

:-Dustin

Changed in lsb:
assignee: nobody → kirkland
importance: Undecided → Medium
status: New → In Progress
Revision history for this message
Dustin Kirkland  (kirkland) wrote :

Patch updated with two minor changes, per Kees...
 * redirect /bin/pidof output to /dev/null
 * use quotes for status="1"

:-Dustin

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package lsb - 3.2-12ubuntu2

---------------
lsb (3.2-12ubuntu2) intrepid; urgency=low

  * init-functions: Replace pidofproc() call in status_of_proc() with a
    /bin/pidof call, to allow any user to query service status (LP: #246735).

 -- Dustin Kirkland <email address hidden> Tue, 08 Jul 2008 15:38:55 -0500

Changed in lsb:
status: In Progress → Fix Released
Revision history for this message
Matt Zimmerman (mdz) wrote :

Why not fix pidofproc itself rather than duplicating the logic?

Revision history for this message
Dustin Kirkland  (kirkland) wrote :

On Wed, Jul 9, 2008 at 6:59 AM, Matt Zimmerman <email address hidden> wrote:
> Why not fix pidofproc itself rather than duplicating the logic?

If a pidfile is available, pidofproc() uses the following to determine if a process is active:
            ...
            if $(kill -0 "${pid:-}" 2> /dev/null); then
                echo "$pid"
                return 0
                return 1 # program is dead and /var/run pid file exists
            fi
            ...

I assume this is to account for zombie processes, where a process exists in the process table, but it will not receive a -0 signal.

The "kill -0" operation is restricted to the owner of the process (and root).

I considered an "else" statement, detecting an "Operation not permitted" error.

But I was concerned that was a serious functional change to the manner in which pidofproc() operates, affecting its many callers. In the interest of getting this patch accepted by Debian (it has been, see Debian Bug #483285), I thought that creating a new function runnable by any user would be the safest approach.

I've attached another patch, that detects an "Operation not permitted" error in the pidofproc() kill call, and returns 100 (in order to preserve a non-zero return code for legacy callers of pidofproc()). This patch does keep a status_of_proc() around, to call pidofproc(), and detect the "0 or 100" return code.

This patch also has a *significant* advantage over its predecssor. Having status_of_proc() call pidofproc() supports pidfiles, something that slangasek and lamont have raised as an issue. I'm subscribing them to this bug for comment.

Note that I chose 100 based on http://refspecs.linux-foundation.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html (100-149 reserved for distribution use).

:-Dustin

Revision history for this message
Matt Zimmerman (mdz) wrote : Re: [Bug 246735] Re: status_of_proc() calls pidofproc() which calls kill, requiring ownership privileges on the process

On Wed, Jul 09, 2008 at 03:16:24PM -0000, Dustin Kirkland wrote:
> If a pidfile is available, pidofproc() uses the following to determine if a process is active:
> ...
> if $(kill -0 "${pid:-}" 2> /dev/null); then
> echo "$pid"
> return 0
> return 1 # program is dead and /var/run pid file exists
> fi
> ...
>
> I assume this is to account for zombie processes, where a process exists
> in the process table, but it will not receive a -0 signal.

Whatever its reasons, it is buggy and should probably be fixed. :-)

> The "kill -0" operation is restricted to the owner of the process (and
> root).

And rightly so. kill(..., 0) is meant to tell the caller whether it's
possible for them to send a signal to the specified PID, without actually
sending any. As is clear from the problem you found, this is not the same
thing as testing whether the process is running.

> I've attached another patch, that detects an "Operation not permitted"
> error in the pidofproc() kill call, and returns 100 (in order to
> preserve a non-zero return code for legacy callers of pidofproc()).
> This patch does keep a status_of_proc() around, to call pidofproc(), and
> detect the "0 or 100" return code.

How about using ps(1) instead of kill(1) to look for the process? That
should work regardless of the caller's ability to send signals, and doesn't
require interpreting the error code.

--
 - mdz

Revision history for this message
Dustin Kirkland  (kirkland) wrote :

Good call, Matt. Steve Langasek made the same recommendation.

Here's an updated patch. Any other comments? If this is acceptable, I'll push to debian too.

:-Dustin

Revision history for this message
Dustin Kirkland  (kirkland) wrote :

Steve recommends *not* returning 100 in the case where the euid doesn't own the process.

pidofproc is not documented as returning non-zero in the case you don't own the process. In fact, if no pid file is found, and /bin/pidof runs farther down, it doesn't check ownership either.

This patch returns 0 if the ps finds a process with a matching pid.

Comments?

:-Dustin

Revision history for this message
Matt Zimmerman (mdz) wrote :

On Wed, Jul 09, 2008 at 07:10:03PM -0000, Dustin Kirkland wrote:
> Steve recommends *not* returning 100 in the case where the euid doesn't
> own the process.
>
> pidofproc is not documented as returning non-zero in the case you don't
> own the process. In fact, if no pid file is found, and /bin/pidof runs
> farther down, it doesn't check ownership either.
>
> This patch returns 0 if the ps finds a process with a matching pid.
>
> Comments?

Sounds sane to me.

I think it would be fine to remove the kill(1) bits entirely, but am not
bothered if you want to leave it in either.

--
 - mdz

Revision history for this message
Steve Langasek (vorlon) wrote : Re: [Bug 246735] Re: status_of_proc() calls pidofproc() which calls kill, requiring ownership privileges on the process

On Wed, Jul 09, 2008 at 07:33:46PM -0000, Matt Zimmerman wrote:
> I think it would be fine to remove the kill(1) bits entirely, but am not
> bothered if you want to leave it in either.

We discussed this and I conceded it would be advantageous to leave it in
because ps is in the procps package, which is not essential; using kill
first and falling back to ps avoids having to grow the dependency graph
given that procps will always be present (albeit not always configured) as
part of the "required" set.

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

Revision history for this message
Dustin Kirkland  (kirkland) wrote :

Updated patch. Added support for passing a -p $pidfile parameter to status_of_proc().

:-Dustin

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers