Project

General

Profile

Bug #2695

[PATCH] server.c: call ftruncate on pid file

Added by mackyle over 1 year ago. Updated about 1 year ago.

Status:
Fixed
Priority:
Normal
Assignee:
Category:
core
Target version:
Start date:
2015-12-03
Due date:
% Done:

100%

Missing in 1.5.x:
Yes

Description

If the server has changed its uid or is running in a chroot
it may be unable to remove the pid file when it exits.

However, if it holds on to an open handle to the pid file
that has write permission, it will be able to truncate the
pid file to 0 bytes in length.

Most monitoring software recognizes a 0-length pid file
as indicating there is no process running.

Therefore always attempt to truncate the pid file before
trying to remove it so that it's not left containing the
pid of a process that is no longer running.

Patch file attached.

See also http://repo.or.cz/lighttpd/svnmirror/patches.git/commitdiff/2586bb88

0009-server.c-call-ftruncate-on-pid-file_patch.txt View (1.77 KB) mackyle, 2015-12-03 23:05

0004-server.c-call-ftruncate-on-pid-file_patch.txt View (3.62 KB) mackyle, 2015-12-05 01:05

0004-server.c-call-ftruncate-on-pid-file_patch.txt View (4.8 KB) mackyle, 2015-12-05 02:19

0004-server.c-call-ftruncate-on-pid-file_patch.txt View (4.27 KB) mackyle, 2016-02-27 21:32

Associated revisions

Revision 3112 (diff)
Added by stbuehler about 1 year ago

[core] truncate pidfile on exit (fixes #2695)

If the server has changed its uid or is running in a chroot
it may be unable to remove the pid file when it exits.

However, if it holds on to an open handle to the pid file
that has write permission, it will be able to truncate the
pid file to 0 bytes in length.

Most monitoring software recognizes a 0-length pid file
as indicating there is no process running.

Therefore always attempt to truncate the pid file before
trying to remove it so that it's not left containing the
pid of a process that is no longer running.

Signed-off-by: Kyle J. McKay <>

Revision c92b1762 (diff)
Added by mackyle about 1 year ago

[core] truncate pidfile on exit (fixes #2695)

If the server has changed its uid or is running in a chroot
it may be unable to remove the pid file when it exits.

However, if it holds on to an open handle to the pid file
that has write permission, it will be able to truncate the
pid file to 0 bytes in length.

Most monitoring software recognizes a 0-length pid file
as indicating there is no process running.

Therefore always attempt to truncate the pid file before
trying to remove it so that it's not left containing the
pid of a process that is no longer running.

Signed-off-by: Kyle J. McKay <>

git-svn-id: svn://svn.lighttpd.net/lighttpd/branches/lighttpd-1.4.x@3112 152afb58-edef-0310-8abb-c4023f1b3aa9

History

#1 Updated by stbuehler over 1 year ago

  • Target version changed from 1.4.x to 1.4.39

Sounds useful; I will need to have a closer look whether:

  • it needs close-on-exec flag
  • what happens in the max-worker spawning case

#2 Updated by mackyle over 1 year ago

I tend to think of lighttpd as not forking, but you're right, it does and it needs close-on-exec.

I have attached an improved patch with close-on-exec and proper graceful shutdown handling (the old patch overlooked that).

Also, in adding the graceful shutdown truncating, I slid the graceful shutdown unlink pid-file code down a few lines to be outside the fd loop so that an attempt is not made to unlink the pid file multiple times producing spurious "unlink failed for: ... 2 No such file or directory" messages.

See also http://repo.or.cz/lighttpd/svnmirror/patches.git/commitdiff/3fdb9db0

As for the max-worker spawning case, the workers ignore the pid file, right? If not, perhaps they should have their srv->srvconf.pid_file cleared and pid_fd set to -1 immediately after the fork that creates them as a worker has no business mucking with the pid file.

#3 Updated by mackyle over 1 year ago

This version of the patch addresses the max-worker spawning case by making the workers' parent responsible for removing the pid-file. After all, shouldn't the process whose pid is recorded in the pid-file be responsible for removing it?

See also http://repo.or.cz/lighttpd/svnmirror/patches.git/commitdiff/c63a20c1

#4 Updated by stbuehler about 1 year ago

  • Target version changed from 1.4.39 to 1.4.40

#5 Updated by gstrauss about 1 year ago

@mackyle, would you please put the repeated code to ftruncate and unlink the pid file into a subroutine?

e.g. void pidfile_cleanup(server *srv, int pid_fd);

#7 Updated by gstrauss about 1 year ago

The latest patch looks good to me. Thanks, @mackyle!

FYI: I'm not sure if @stbuehler prefers patches attached to these tickets or github pull requests. I have been doing both for my own requests with tested patches.

#8 Updated by stbuehler about 1 year ago

Yes, thanks @mackyle for the patch and @gstrauss for the review help.

I just changed a small part: I don't like single-statement "blocks" on a separate line - either same line (if (bla) return foo;) or add { ... }.

#9 Updated by stbuehler about 1 year ago

  • Status changed from New to Fixed
  • % Done changed from 0 to 100

Applied in changeset r3112.

Also available in: Atom