When SMTP server return an erreneous response upon QUIT message, socket is not closed and leaks

[pounard]

Leaking opened TCP socket which remain open as long as the PHP process lives. This especially cause problems when running applications such as long running message bus consumers that sporadically send mails: if you open a new socket per mail, eventually you'll reach a point where you deplete allowed OS open file limit (ulimit).

I'll debug on my side, we reproduced a specific scenario where the server doesn't answer correctly on QUIT command, and I'll open a PR.

[pounard]

Culprit code is:

    public function disconnect()
    {
        if (PEAR::isError($error = $this->put('QUIT'))) {
            return $error;
        }
        if (PEAR::isError($error = $this->parseResponse(221))) {
            return $error;
        }
        if (PEAR::isError($error = $this->socket->disconnect())) {
            return PEAR::raiseError(
                'Failed to disconnect socket: ' . $error->getMessage()
            );
        }

        return true;
    }

I think it should not return early in case of error. Either copy the variable and attempt socket disconnect before exiting, or use a try/finally statement and disconnect the socket forcefully within the finally, and it should solve the problem.

[pounard]

For those interested, my (ulgy) manual fix in a custom project, without the need of patching the library (beware, here lies dragons):

    public send(SomeMailDataStructure $someMail): void
    {
        $smtp = null;
        try {
            $smtp = new \Net_SMTP(/* Params... */);

            $result = $smtp->connect();
            $this->throwExceptionOnError($result, $smtp);

            $this->doSend($someMail, $smtp); /* Real mail send, not relevant here. */

        } finally {
            // Here lies the code that does it.
            if ($smtp) {
                $result = $smtp->disconnect();

                // Disconnect can silently leave the SMTP connection opened.
                // Try to forcefully close it.
                // This is hardcode magic, kids, don't do this at home.
                $foo = \Closure::bind(static function (\Net_SMTP $smtp) {
                    return $smtp->socket;
                }, null, \Net_SMTP::class);

                if (($socket = $foo($smtp)) && $socket instanceof \Net_Socket) {
                    $socket->disconnect();
                }

                // Do not raise any error here, sometime some SMTP servers will
                // return invalid response codes upon the QUIT SMTP command, but
                // we do not care, we are just going to close the connection
                // anyway.
                if (\PEAR::isError($result)) {
                    // @todo In 2.0 here instrument error.
                }
            }
        }
    }

[pounard]

I think it should not return early in case of error. Either copy the variable and attempt socket disconnect before exiting, or use a try/finally statement and disconnect the socket forcefully within the finally, and it should solve the problem.

Or another solution that would not imply a behavioural change of this API: provide an additional forceDisconnect() method that would close the socket if opened anyway without further ado.

[alecpl]

I agree that this should be a default behavior of disconnect(). Maybe instead of adding forceDisconnect() we should add quit() to make these few users that need the QUIT result happy.

[pounard]

I like this solution, seems perfect, I'll try to take some time to make a PR but I can't promise any deadline, I have much work beside.

[jparise]

The disconnect() method is currently documented as "Attempt to disconnect from the SMTP server.", and I think the current code puts an overly-strong emphasis on the "attempt" part.

Another idea would be to add an optional $force parameter that would ensure the socket was disconnect for all of the error cases, too.

--- a/Net/SMTP.php
+++ b/Net/SMTP.php
@@ -489,12 +489,18 @@ class Net_SMTP
      *               kind of failure, or true on success.
      * @since 1.0
      */
-    public function disconnect()
+    public function disconnect($force = false)
     {
         if (PEAR::isError($error = $this->put('QUIT'))) {
+            if ($force) {
+              $this->socket->disconnect();
+            }
             return $error;
         }
         if (PEAR::isError($error = $this->parseResponse(221))) {
+            if ($force) {
+              $this->socket->disconnect();
+            }
             return $error;
         }
         if (PEAR::isError($error = $this->socket->disconnect())) {

[schengawegga]

I created a pull request based on @jparise idea