Project

General

Profile

Actions

Bug #2695

closed

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

Added by mackyle over 8 years ago. Updated about 8 years ago.

Status:
Fixed
Priority:
Normal
Category:
core
Target version:
ASK QUESTIONS IN Forums:

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


Files

Actions #1

Updated by stbuehler over 8 years 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
Actions #2

Updated by mackyle over 8 years 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.

Actions #3

Updated by mackyle over 8 years 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

Actions #4

Updated by stbuehler about 8 years ago

  • Target version changed from 1.4.39 to 1.4.40
Actions #5

Updated by gstrauss about 8 years 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);

Actions #7

Updated by gstrauss about 8 years 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.

Actions #8

Updated by stbuehler about 8 years 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 { ... }.

Actions #9

Updated by stbuehler about 8 years ago

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

Applied in changeset r3112.

Actions

Also available in: Atom