Skip to content

php_readpipe() and php_init() recurse without bound when a script server starts but never answers #608

Description

@somethingwithproof

php_readpipe() restarts the script server from inside itself, and php_init() calls php_readpipe() to confirm the new server started. Nothing bounds the depth, so a server that starts but never answers puts a poller thread into unbounded mutual recursion.

The cycle, on develop:

php_readpipe()                       php.c:166
  ... select() times out or fails
  php_close(php_process)             php.c:244, 254
  php_init(php_process)              php.c:245, 255
      php_readpipe(php_process, ...) php.c:485, 487   <- back to the top

php_cmd() enters the same cycle at php.c:94 when the write fails.

A spawn failure terminates it: php_init() returns before reaching the handshake read, so the recursion unwinds. The case that does not terminate is a script server that spawns successfully and then never responds. Each level times out after script_timeout, kills that server, starts another, and recurses. The thread never returns, and a fresh script server process is spawned per level.

Ways to get there: a script server that blocks on startup because of a misconfigured Cacti include path or an unreachable database, a path_php_server pointing at something that runs but is not the script server, or an interpreter that starts and then hangs on its own I/O.

Observed while adding coverage for php.c. Forcing select() to fail persistently reproduces it immediately, since the failure is instant rather than delayed by the timeout; the test binary consumed its whole 240 second budget without completing a single case.

Two shapes for a fix, and which is right is a judgement about restart policy rather than something to pick by inspection:

  1. Bound the depth. A thread-local counter incremented across the php_close() / php_init() pair, returning U rather than recursing past one or two attempts.
  2. Do not restart from inside the read at all. php_readpipe() marks the server unusable and returns U, and the restart happens at the next poll from a caller that is not already inside this path.

The second is closer to what the code seems to intend, since php_cmd() already has its own retry budget for the write side, but it changes when a server is replaced and deserves a maintainer's call.

Present on develop and 1.2.x.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions