problem with start-stop-daemon and --exec

[Denys Vlasenko]

On Mon, Mar 5, 2012 at 4:25 PM, Ed W <lists at wildgooses.com> wrote:
>> But there is a reason why we check /proc/pid/cmdline instead.
>> On systems with bbox, a lot of processes will have
>> /proc/pid/exe = "/bin/busybox".
>> Which will make e.g.
>> start-stop-daemon --stop --exec /bin/ntpd
>> fail to stop ntpd if ntpd is a busybox applet.
>
> I examined the code for openrc and I can't find any obvious reference to
> /proc/pid/exe ?  I confess I don't understand the implementation they are
> using, but I think the relevant code is in src/librc/librc-daemon.c if
> someone else could take a closer look?

http://sources.gentoo.org/cgi-bin/viewvc.cgi/baselayout/trunk/src/librc-daemon.c?view=markup&pathrev=2966

45 	snprintf (cmdline, sizeof (cmdline), "/proc/%u/exe", pid);
46 	memset (buffer, 0, sizeof (buffer));
47 	if (readlink (cmdline, buffer, sizeof (buffer)) != -1) {
48 	if (strcmp (exec, buffer) == 0)
49 	return (true);

Heh...

66 	r = read(fd, buffer, sizeof (buffer));
67 	close (fd);
68 	
69 	if (r == -1)
70 	return 0;
71 	
72 	buffer[r] = 0;
^^^^^^^^^^^^^^^^^^^^^^^^^^ buffer overflow

> Can we please consider simply relaxing the current test to be a substring
> match on /proc/pid/cmdline (only in the case of both --exec AND --pidfile)?
>  This seems to
> a) be a small code change and completely compatible with current situation
> b) still a very tight test and unlikely to lead to false positives
> c) handles common situations where a daemon renames itself

I don't fully understand. Can you show a patch?

So far I committed this:

http://git.busybox.net/busybox/commit/?id=17eedcad9406c43beddab3906c8c693626c351fb

-- 
vda