From 1a9257c900ed55cbb7c1fcee75311d6e60cdeb06 Mon Sep 17 00:00:00 2001
From: Andreas Gohr <andi@splitbrain.org>
Date: Sun, 28 Jun 2026 17:49:20 +0200
Subject: [PATCH] make SMTP socket I/O robust against multi-line replies and
 timeouts
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

getCode() now reads each reply line with a length-independent readLine() and
reassembles a multi-line reply (RFC 5321 §4.2.1) into a single resultStack
entry. The previous reader used fgets($smtp, 515): it truncated any line over
514 bytes and left the tail buffered, and detected end-of-reply from a byte at
a fixed offset. Both desynced the stream, so an error could be reported against
the wrong command even when the message was actually delivered. Recording one
reassembled reply per command also realigns resultStack with commandStack, so
CodeException reports the whole reply rather than just its final line.

Also add a configurable socket timeout (Mailer/SMTP::setTimeout) applied to
both connect and reads, report read timeouts distinctly, and fail loudly on a
write error in pushStack.

Add unit tests for the reply parser (single/multi-line, long >buffer lines,
back-to-back replies, error cases) driven by an in-memory stream.
---
 src/Mailer.php           |  12 +++
 src/Mailer/SMTP.php      |  98 ++++++++++++++++++++---
 tests/SMTPParserTest.php | 168 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 265 insertions(+), 13 deletions(-)
 create mode 100644 tests/SMTPParserTest.php

diff --git a/src/Mailer.php b/src/Mailer.php
index eab1b6d..f09b132 100644
--- a/src/Mailer.php
+++ b/src/Mailer.php
@@ -67,6 +67,18 @@ public function setServer($host, $port, $secure=null)
         return $this;
     }
 
+    /**
+     * set the socket timeout in seconds, applied to both connecting and
+     * to each read/write while talking to the server
+     * @param int $timeout
+     * @return $this
+     */
+    public function setTimeout($timeout)
+    {
+        $this->smtp->setTimeout($timeout);
+        return $this;
+    }
+
     /**
      * auth with server
      * @param string $username
diff --git a/src/Mailer/SMTP.php b/src/Mailer/SMTP.php
index 50bc3c1..65c697a 100644
--- a/src/Mailer/SMTP.php
+++ b/src/Mailer/SMTP.php
@@ -41,6 +41,11 @@ class SMTP
      */
     protected $port;
 
+    /**
+     * socket timeout in seconds (connect + read/write)
+     */
+    protected $timeout;
+
     /**
      * smtp secure ssl tls tlsv1.0 tlsv1.1 tlsv1.2
      */
@@ -102,6 +107,7 @@ class SMTP
     public function __construct(LoggerInterface $logger=null)
     {
         $this->logger = $logger;
+        $this->timeout = (int) ini_get('default_socket_timeout');
     }
 
     /**
@@ -160,6 +166,18 @@ public function setEhlo($ehlo)
         return $this;
     }
 
+    /**
+     * set the socket timeout in seconds, applied to both connecting and
+     * to each read/write while talking to the server
+     * @param int $timeout
+     * @return $this
+     */
+    public function setTimeout($timeout)
+    {
+        $this->timeout = (int) $timeout;
+        return $this;
+    }
+
     /**
      * Send the message
      *
@@ -219,13 +237,14 @@ protected function connect()
             $host.':'.$this->port,
             $error_code,
             $error_message,
-            ini_get('default_socket_timeout'),
+            $this->timeout,
             STREAM_CLIENT_CONNECT,
             $context
         );
         if (!$this->smtp){
             throw new SMTPException("Could not open SMTP Port to $host:{$this->port}");
         }
+        stream_set_timeout($this->smtp, $this->timeout);
         $code = $this->getCode();
         if ($code !== '220'){
             throw new CodeException('220', $code, array_pop($this->resultStack));
@@ -449,28 +468,81 @@ protected function quit()
     protected function pushStack($string)
     {
         $this->commandStack[] = $string;
-        fputs($this->smtp, $string, strlen($string));
+        if (fputs($this->smtp, $string, strlen($string)) === false) {
+            throw new SMTPException("Failed to write to SMTP socket");
+        }
         $this->logger && $this->logger->debug('Sent: '. $string);
         return $this->getCode();
     }
 
     /**
-     * get smtp response code
-     * once time has three digital and a space
-     * @return string
-     * @throws SMTPException
+     * Read one complete SMTP reply and return its 3-digit status code.
+     *
+     * An SMTP reply may span several lines (RFC 5321 §4.2.1): continuation
+     * lines have the form "NNN-text", the final line "NNN text" (or "NNN").
+     * All lines are read and reassembled into a single reply, which is pushed
+     * as one entry onto $this->resultStack -- so it stays aligned with
+     * $this->commandStack (one result per command) and CodeException can
+     * surface the whole reply, not just its last line.
+     *
+     * @return string the 3-digit reply code
+     * @throws SMTPException on a closed/timed-out connection or an unparseable reply
      */
     protected function getCode()
     {
-        while ($str = fgets($this->smtp, 515)) {
-            $this->logger && $this->logger->debug("Got: ". $str);
-            $this->resultStack[] = $str;
-            if(substr($str,3,1) == " ") {
-                $code = substr($str,0,3);
-                return $code;
+        $code = null;
+        $reply = '';
+
+        do {
+            $line = $this->readLine();
+            if ($line === null) {
+                $meta = stream_get_meta_data($this->smtp);
+                if (!empty($meta['timed_out'])) {
+                    throw new SMTPException("SMTP read timed out after {$this->timeout} seconds");
+                }
+                throw new SMTPException("SMTP Server did not respond with anything I recognized");
+            }
+
+            $this->logger && $this->logger->debug("Got: " . $line);
+            $reply .= $line;
+
+            // RFC 5321 §4.2.1: "NNN " (or bare "NNN") is the final line,
+            // "NNN-" marks a continuation line that must be followed by more.
+            if (!preg_match('/^(\d{3})([ -]?)/', $line, $m)) {
+                $this->resultStack[] = $reply;
+                throw new SMTPException("SMTP Server did not respond with anything I recognized");
+            }
+
+            $code      = $m[1];
+            $separator = $m[2];
+        } while ($separator === '-');
+
+        $this->resultStack[] = $reply;
+
+        return $code;
+    }
+
+    /**
+     * Read a single complete line (terminated by LF) from the socket,
+     * regardless of how long it is or how the OS chunks it. Reassembling the
+     * whole line here is what keeps a long reply from leaving a tail in the
+     * buffer that would desync the next getCode() call.
+     *
+     * @return string|null the line (including its trailing CRLF), or null on EOF/timeout
+     */
+    protected function readLine()
+    {
+        $line = '';
+        while (true) {
+            $chunk = fgets($this->smtp, 1024);
+            if ($chunk === false) {
+                return ($line === '') ? null : $line;   // EOF / timeout
+            }
+            $line .= $chunk;
+            if (substr($line, -1) === "\n") {
+                return $line;                            // got the full line
             }
         }
-        throw new SMTPException("SMTP Server did not respond with anything I recognized");
     }
 
 }
diff --git a/tests/SMTPParserTest.php b/tests/SMTPParserTest.php
new file mode 100644
index 0000000..450deb6
--- /dev/null
+++ b/tests/SMTPParserTest.php
@@ -0,0 +1,168 @@
+<?php
+
+namespace Tx\Mailer\Tests;
+
+use PHPUnit\Framework\TestCase as BaseTestCase;
+use Tx\Mailer\SMTP;
+use Tx\Mailer\Exceptions\SMTPException;
+
+/**
+ * Unit tests for SMTP::getCode() / readLine() reply parsing (no network).
+ *
+ * The parser reads from the protected $smtp socket, so each test wires a real
+ * SMTP instance to an in-memory stream and invokes the protected getCode() via
+ * reflection — that keeps us testing the real implementation without a helper
+ * subclass and without stubbing anything away.
+ */
+class SMTPParserTest extends BaseTestCase
+{
+    /**
+     * Build a real SMTP whose socket is an in-memory stream preloaded with the
+     * given reply, and return: a callable that invokes the protected getCode(),
+     * the stream itself (to assert how much was consumed), and a callable that
+     * returns the protected resultStack (the reassembled replies CodeException
+     * reports on failure).
+     *
+     * @return array{0: callable, 1: resource, 2: callable}
+     */
+    private function smtpReading($reply)
+    {
+        $fp = fopen('php://memory', 'r+');
+        fwrite($fp, $reply);
+        rewind($fp);
+
+        $smtp = new SMTP();
+
+        $prop = new \ReflectionProperty(SMTP::class, 'smtp');
+        $getCode = new \ReflectionMethod(SMTP::class, 'getCode');
+        $stack = new \ReflectionProperty(SMTP::class, 'resultStack');
+
+        // setAccessible() is a no-op since PHP 8.1 and a deprecation as of 8.5,
+        // so only call it on the older PHP versions where it is still required.
+        if (PHP_VERSION_ID < 80100) {
+            $prop->setAccessible(true);
+            $getCode->setAccessible(true);
+            $stack->setAccessible(true);
+        }
+
+        $prop->setValue($smtp, $fp);
+
+        $invoke = function () use ($getCode, $smtp) {
+            return $getCode->invoke($smtp);
+        };
+        $resultStack = function () use ($stack, $smtp) {
+            return $stack->getValue($smtp);
+        };
+
+        return array($invoke, $fp, $resultStack);
+    }
+
+    public function testSingleLineReply()
+    {
+        list($getCode) = $this->smtpReading("250 OK\r\n");
+        $this->assertSame('250', $getCode());
+    }
+
+    /**
+     * Every continuation line is consumed, leaving the stream at EOF so the
+     * next command reads its own reply.
+     */
+    public function testMultiLineReplyIsFullyConsumed()
+    {
+        list($getCode, $fp) = $this->smtpReading(
+            "250-mx.example.com\r\n" .
+            "250-PIPELINING\r\n" .
+            "250-SIZE 1000000\r\n" .
+            "250 8BITMIME\r\n"
+        );
+        $this->assertSame('250', $getCode());
+        $this->assertFalse(fgets($fp));
+    }
+
+    /**
+     * A multi-line reply is recorded as a single reassembled resultStack entry,
+     * keeping resultStack aligned with commandStack (one result per command).
+     */
+    public function testMultiLineReplyIsReassembledIntoOneStackEntry()
+    {
+        $reply =
+            "250-mx.example.com\r\n" .
+            "250-PIPELINING\r\n" .
+            "250-SIZE 1000000\r\n" .
+            "250 8BITMIME\r\n";
+        list($getCode, , $resultStack) = $this->smtpReading($reply);
+        $this->assertSame('250', $getCode());
+
+        $stack = $resultStack();
+        $this->assertCount(1, $stack);
+        $this->assertSame($reply, $stack[0]);
+    }
+
+    /**
+     * The whole multi-line reply is preserved in the single resultStack entry
+     * callers pop for CodeException, so an explanatory line survives alongside
+     * the final one.
+     */
+    public function testMultiLineErrorReplyKeepsExplanatoryText()
+    {
+        $reply =
+            "535-5.7.8 Username and Password not accepted.\r\n" .
+            "535 5.7.8 https://support.google.com/mail/?p=BadCredentials\r\n";
+        list($getCode, , $resultStack) = $this->smtpReading($reply);
+        $this->assertSame('535', $getCode());
+
+        $stack = $resultStack();
+        $popped = array_pop($stack);
+        $this->assertSame($reply, $popped);
+        $this->assertStringContainsString('Username and Password not accepted', $popped);
+    }
+
+    /**
+     * A reply line longer than readLine()'s fgets() chunk is reassembled
+     * byte-for-byte instead of being truncated, leaving nothing buffered.
+     */
+    public function testLongLineIsNotTruncated()
+    {
+        // > the 1024-byte fgets() chunk, so reassembly must stitch >1 chunk
+        $line = "250 " . str_repeat("X", 2000) . "\r\n";
+        list($getCode, $fp, $resultStack) = $this->smtpReading($line);
+        $this->assertSame('250', $getCode());
+        $captured = $resultStack();
+        $this->assertSame($line, $captured[0]);
+        $this->assertFalse(fgets($fp));
+    }
+
+    /**
+     * After a line longer than the fgets() chunk is reassembled, the stream is
+     * left at the start of the next reply, so the following command reads its
+     * own code.
+     */
+    public function testTwoRepliesStayInSyncAfterLongLine()
+    {
+        // first line > the 1024-byte fgets() chunk, forcing reassembly
+        $first = "250 " . str_repeat("X", 2000) . "\r\n";
+        list($getCode) = $this->smtpReading($first . "354 go ahead\r\n");
+        $this->assertSame('250', $getCode());
+        $this->assertSame('354', $getCode());
+    }
+
+    public function testBareCodeLine()
+    {
+        list($getCode) = $this->smtpReading("220\r\n");
+        $this->assertSame('220', $getCode());
+    }
+
+    public function testEmptyReplyThrows()
+    {
+        list($getCode) = $this->smtpReading("");
+        $this->expectException(SMTPException::class);
+        $getCode();
+    }
+
+    public function testGarbageReplyThrows()
+    {
+        list($getCode) = $this->smtpReading("not an smtp reply\r\n");
+        $this->expectException(SMTPException::class);
+        $getCode();
+    }
+}