diff --git a/CHANGELOG.md b/CHANGELOG.md --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,18 @@ +# v1.0.9.0 + +## Non-breaking + +- Added a `connectdb` spec covering a host name that DNS cannot resolve, asserting `errorMessage` exactly against the reference. Found `pqi-native`'s failure message is the raw `Show`n `getAddrInfo` exception, worded unrelatedly to libpq's own (locale-translated) sentence for the same failure, so a downstream classifier that pattern-matches libpq's wording reacts inconsistently between the two adapters. Found via `hasql` issue #329. + +- Added a `connectdb` spec covering a handshake-time connection reset (`ECONNRESET`, forced via `SO_LINGER` 0) as distinct from a clean EOF. Found `pqi-native`'s `handshakeFailureMessage` only special-cases EOF with libpq-matching wording ("server closed the connection unexpectedly") and falls back to the raw `Show`n `IOException` (e.g. `Network.Socket.recvBuf: resource vanished (Connection reset by peer)`) for any other handshake-time I/O error, even though libpq itself reports a hard reset with the very same EOF wording. Found via `hasql` issue #329. + +- Added an `exec` spec covering a connection reset while a query response is in flight, after a valid handshake. Found none of `pqi-native`'s query functions (`exec`, `execParams`, `prepare`, `execPrepared`, `describePrepared`, `describePortal`) wrap their read loop in any exception handler at all, so a connection lost mid-query escapes as a raw, uncaught `IOException` instead of any classified failure - unlike libpq, which returns a `FatalError` result with its own "server closed the connection unexpectedly" message and never throws. Found via `hasql` issue #329. + # v1.0.8.0 ## Non-breaking -- Added a `connectdb` spec covering a Unix-socket directory that doesn't exist, asserting `errorMessage` byte-for-byte against the reference. Found `pqi-native`'s failure message is the raw `Show`n `IOException`, which happens to contain `"could not connect to server"` - one of `Hasql.Connection`'s networking (transient) patterns - while libpq's message for the identical `ENOENT` doesn't, so the two adapters drive `Hasql.Connection.acquire` to opposite classifications of the same failure. Found via `hasql` issue #329. +- Added a `connectdb` spec covering a Unix-socket directory that doesn't exist, asserting `errorMessage` exactly against the reference. Found `pqi-native`'s failure message is the raw `Show`n `IOException`, which happens to contain `"could not connect to server"` - one of `Hasql.Connection`'s networking (transient) patterns - while libpq's message for the identical `ENOENT` doesn't, so the two adapters drive `Hasql.Connection.acquire` to opposite classifications of the same failure. Found via `hasql` issue #329. # v1.0.7.0 diff --git a/pqi-conformance.cabal b/pqi-conformance.cabal --- a/pqi-conformance.cabal +++ b/pqi-conformance.cabal @@ -1,6 +1,6 @@ cabal-version: 3.0 name: pqi-conformance -version: 1.0.8.0 +version: 1.0.9.0 category: Database, PostgreSQL, Testing synopsis: Differential conformance tests for pqi adapters description: @@ -94,9 +94,11 @@ Pqi.Conformance.Operation.CmdStatus Pqi.Conformance.Operation.CmdTuples Pqi.Conformance.Operation.Connectdb + Pqi.Conformance.Operation.Connectdb.HandshakeReset Pqi.Conformance.Operation.Connectdb.MissingUnixSocketDirectory Pqi.Conformance.Operation.Connectdb.Rejection Pqi.Conformance.Operation.Connectdb.UnixSocketUri + Pqi.Conformance.Operation.Connectdb.UnresolvableHost Pqi.Conformance.Operation.ConnectionNeedsPassword Pqi.Conformance.Operation.ConnectionUsedPassword Pqi.Conformance.Operation.ConnectPoll @@ -113,6 +115,7 @@ Pqi.Conformance.Operation.EscapeIdentifier Pqi.Conformance.Operation.EscapeStringConn Pqi.Conformance.Operation.Exec + Pqi.Conformance.Operation.Exec.ConnectionLostMidQuery Pqi.Conformance.Operation.ExecParams Pqi.Conformance.Operation.ExecPrepared Pqi.Conformance.Operation.ExitPipelineMode diff --git a/src/library/Pqi/Conformance/Operation/Connectdb.hs b/src/library/Pqi/Conformance/Operation/Connectdb.hs --- a/src/library/Pqi/Conformance/Operation/Connectdb.hs +++ b/src/library/Pqi/Conformance/Operation/Connectdb.hs @@ -14,9 +14,11 @@ import qualified Pqi import Pqi.Conformance.Harness import Pqi.Conformance.Observation +import qualified Pqi.Conformance.Operation.Connectdb.HandshakeReset as HandshakeReset import qualified Pqi.Conformance.Operation.Connectdb.MissingUnixSocketDirectory as MissingUnixSocketDirectory import qualified Pqi.Conformance.Operation.Connectdb.Rejection as Rejection import qualified Pqi.Conformance.Operation.Connectdb.UnixSocketUri as UnixSocketUri +import qualified Pqi.Conformance.Operation.Connectdb.UnresolvableHost as UnresolvableHost import Pqi.Conformance.Prelude import qualified Pqi.Conformance.Reference as Reference import Test.Hspec @@ -27,6 +29,8 @@ Rejection.spec adapter UnixSocketUri.spec adapter MissingUnixSocketDirectory.spec adapter + UnresolvableHost.spec adapter + HandshakeReset.spec adapter describe "connectdb" do it "opens a usable connection" \conninfo -> diff --git a/src/library/Pqi/Conformance/Operation/Connectdb/HandshakeReset.hs b/src/library/Pqi/Conformance/Operation/Connectdb/HandshakeReset.hs new file mode 100644 --- /dev/null +++ b/src/library/Pqi/Conformance/Operation/Connectdb/HandshakeReset.hs @@ -0,0 +1,111 @@ +-- | Coverage for a handshake-time failure that is a hard reset +-- (@ECONNRESET@) rather than a clean EOF: the server accepts the TCP +-- connection, reads the startup packet, then tears the socket down with +-- @SO_LINGER@ set to a zero timeout so the close sends a TCP @RST@ instead +-- of the usual @FIN@. +-- +-- Unlike a graceful close (see +-- 'Pqi.Conformance.Operation.Connectdb.Rejection', which truncates an +-- @ErrorResponse@ frame and closes normally), a reset is a genuinely +-- different I\/O failure shape, and was not routed through the same code +-- path in @pqi-native@: +-- 'Pqi.Native.Connection.handshakeFailureMessage' special-cased an EOF +-- (\"server closed the connection unexpectedly\", matching libpq's own +-- wording) but fell back to the raw 'Show'n 'System.IO.Error.IOException' +-- for anything else - and @ECONNRESET@ landed in that \"anything else\" +-- branch. +-- +-- Was found in @pqi-native@ +-- (): the candidate's +-- 'Pqi.errorMessage' for this failure was e.g. @Network.Socket.recvBuf: +-- resource vanished (Connection reset by peer)@, entirely unlike libpq's +-- own \"server closed the connection unexpectedly\" sentence for the same +-- underlying reset. Fixed by widening +-- 'Pqi.Native.Connection.handshakeFailureMessage's classification to +-- also recognize a hard reset (@ioe_type == ResourceVanished@), not just +-- a clean EOF. +module Pqi.Conformance.Operation.Connectdb.HandshakeReset + ( spec, + ) +where + +import Control.Concurrent (forkIO) +import Control.Exception (SomeException, bracket, try) +import qualified Data.ByteString as ByteString +import qualified Data.ByteString.Char8 as ByteString.Char8 +import qualified Network.Socket as Socket +import qualified Network.Socket.ByteString as Socket.ByteString +import qualified Pqi +import Pqi.Conformance.Prelude +import qualified Pqi.Conformance.Reference as Reference +import Test.Hspec + +spec :: Pqi.Adapter -> SpecWith ByteString +spec adapter = + describe "connectdb" do + describe "a handshake-time connection reset (not a clean EOF)" do + it "the candidate reports a failure message shaped like the reference's" \_ -> + -- Both attempts share one listener (and so one port): the failure + -- message embeds the port number, and the candidate and reference + -- would otherwise always disagree on that one detail despite + -- matching in every way that matters. + withResettingServer \port -> do + candidate <- attempt adapter port + reference <- attempt Reference.adapter port + candidate `shouldBe` reference + +-- | Run 'Pqi.connectdb' against the given port (see 'withResettingServer') +-- and report the resulting status and error message, or the exception's +-- 'Show'n form if one escaped - which is exactly what should never happen. +-- +-- @sslmode=disable@ keeps this comparable across adapters: libpq negotiates +-- SSL before the startup packet by default, so without it the reset would +-- land during a preamble @pqi-native@ (which never attempts SSL) does not +-- even send. The host is given as a literal IP rather than a name so +-- libpq's failure message doesn't gain a resolved-IP parenthetical the +-- candidate would then also have to reproduce. +attempt :: Pqi.Adapter -> Socket.PortNumber -> IO (Either String (Pqi.ConnStatus, Maybe ByteString)) +attempt adapter port = do + let conninfo = + "host=127.0.0.1 port=" + <> ByteString.Char8.pack (show port) + <> " dbname=x user=x sslmode=disable" + result <- try @SomeException (Pqi.connectdb adapter conninfo) + case result of + Left err -> pure (Left (show err)) + Right connection -> do + observedStatus <- Pqi.status connection + observedError <- Pqi.errorMessage connection + Pqi.finish connection + pure (Right (observedStatus, observedError)) + +-- | Bind a loopback listener on an ephemeral port and hand its port number +-- to the action, while a background thread serves every connection made to +-- it in turn: read whatever the client has sent so far (the startup +-- packet), then reset the connection - via @SO_LINGER@ with a zero timeout, +-- which makes the kernel send a @RST@ instead of the usual @FIN@ on close - +-- rather than closing it gracefully. +withResettingServer :: (Socket.PortNumber -> IO a) -> IO a +withResettingServer action = + bracket open Socket.close \listener -> do + port <- Socket.socketPort listener + _ <- forkIO (try @SomeException (forever (serveOneReset listener)) >> pure ()) + action port + where + open = do + address : _ <- + Socket.getAddrInfo + (Just Socket.defaultHints {Socket.addrSocketType = Socket.Stream}) + (Just "127.0.0.1") + (Just "0") + sock <- Socket.socket (Socket.addrFamily address) (Socket.addrSocketType address) (Socket.addrProtocol address) + Socket.bind sock (Socket.addrAddress address) + Socket.listen sock 8 + pure sock + +serveOneReset :: Socket.Socket -> IO () +serveOneReset listener = do + (conn, _) <- Socket.accept listener + _ <- Socket.ByteString.recv conn 4096 + Socket.setSockOptValue conn Socket.Linger (Socket.SockOptValue (Socket.StructLinger 1 0)) + Socket.close conn diff --git a/src/library/Pqi/Conformance/Operation/Connectdb/MissingUnixSocketDirectory.hs b/src/library/Pqi/Conformance/Operation/Connectdb/MissingUnixSocketDirectory.hs --- a/src/library/Pqi/Conformance/Operation/Connectdb/MissingUnixSocketDirectory.hs +++ b/src/library/Pqi/Conformance/Operation/Connectdb/MissingUnixSocketDirectory.hs @@ -6,7 +6,7 @@ -- @ENOENT@ - so unlike -- 'Pqi.Conformance.Operation.Connectdb.UnixSocketUri' (whose two adapters -- fail via genuinely unrelated machinery: DNS resolution vs. a filesystem --- check), this spec does compare 'Pqi.errorMessage' byte-for-byte. It's +-- check), this spec does compare 'Pqi.errorMessage' exactly. It's -- expected to fail on @pqi-native@ right now, and deliberately left that -- way: the mismatch it demonstrates is exactly what confuses downstream -- string-matching classifiers. diff --git a/src/library/Pqi/Conformance/Operation/Connectdb/UnixSocketUri.hs b/src/library/Pqi/Conformance/Operation/Connectdb/UnixSocketUri.hs --- a/src/library/Pqi/Conformance/Operation/Connectdb/UnixSocketUri.hs +++ b/src/library/Pqi/Conformance/Operation/Connectdb/UnixSocketUri.hs @@ -14,7 +14,7 @@ -- 'Pqi.errorMessage': both adapters fail to connect either way (there is no -- such socket directory), but their failure text is produced by unrelated, -- adapter-specific machinery (DNS resolution vs. a filesystem check) and --- isn't expected to match byte-for-byte. What must match is the host each +-- isn't expected to match exactly. What must match is the host each -- adapter parsed out of the conninfo, which is deterministic and -- OS\/locale-independent. module Pqi.Conformance.Operation.Connectdb.UnixSocketUri diff --git a/src/library/Pqi/Conformance/Operation/Connectdb/UnresolvableHost.hs b/src/library/Pqi/Conformance/Operation/Connectdb/UnresolvableHost.hs new file mode 100644 --- /dev/null +++ b/src/library/Pqi/Conformance/Operation/Connectdb/UnresolvableHost.hs @@ -0,0 +1,44 @@ +-- | Coverage for @connectdb@ against a host name that DNS cannot resolve. +-- +-- Was found in @pqi-native@ +-- (): the candidate's +-- 'Pqi.errorMessage' was the raw 'Show'n form of the underlying +-- 'Network.Socket.getAddrInfo' exception (@could not connect to server: +-- Network.Socket.getAddrInfo (called with preferred socket +-- type\/protocol: ...): does not exist (Name or service not known)@), +-- whereas libpq's own message for the identical failure is its own, +-- differently-worded (and locale-translated) sentence (@could not +-- translate host name "..." to address: Name or service not known@). +-- Both mention the resolver failure, but in unrelated phrasing, so a +-- downstream classifier that pattern-matches on libpq's wording (e.g. +-- @Hasql.Connection@'s networking-error patterns) reacted differently to +-- the two adapters for what is otherwise the same underlying failure. +-- Fixed in @pqi-native@'s @Pqi.Native.Connection.connectFailureMessage@, +-- which now recognizes a resolver failure by its 'ioe_location' and +-- reproduces libpq's own wording for it. +module Pqi.Conformance.Operation.Connectdb.UnresolvableHost + ( spec, + ) +where + +import qualified Pqi +import Pqi.Conformance.Harness +import Pqi.Conformance.Prelude +import Test.Hspec + +spec :: Pqi.Adapter -> SpecWith ByteString +spec adapter = + describe "connectdb" do + describe "a host name that DNS cannot resolve" do + it "reports a failure message shaped like the reference's" \_ -> + differentialConnect adapter conninfo \adapter' conninfo' -> do + connection <- Pqi.connectdb adapter' conninfo' + observedStatus <- Pqi.status connection + observedError <- Pqi.errorMessage connection + Pqi.finish connection + pure (observedStatus, observedError) + where + -- A hostname reserved by RFC 2606 conventions for never resolving: + -- this test is only about the shape of the failure, not about a real + -- connection succeeding. + conninfo = "host=nonexistent.invalid.host" diff --git a/src/library/Pqi/Conformance/Operation/Exec.hs b/src/library/Pqi/Conformance/Operation/Exec.hs --- a/src/library/Pqi/Conformance/Operation/Exec.hs +++ b/src/library/Pqi/Conformance/Operation/Exec.hs @@ -7,12 +7,15 @@ import qualified Pqi import Pqi.Conformance.Harness +import qualified Pqi.Conformance.Operation.Exec.ConnectionLostMidQuery as ConnectionLostMidQuery import Pqi.Conformance.Prelude import Pqi.Conformance.Scenario import Test.Hspec spec :: Pqi.Adapter -> SpecWith ByteString -spec adapter = +spec adapter = do + ConnectionLostMidQuery.spec adapter + describe "exec" do let forCase title sql = it title \conninfo -> differential adapter conninfo (execScenario sql) diff --git a/src/library/Pqi/Conformance/Operation/Exec/ConnectionLostMidQuery.hs b/src/library/Pqi/Conformance/Operation/Exec/ConnectionLostMidQuery.hs new file mode 100644 --- /dev/null +++ b/src/library/Pqi/Conformance/Operation/Exec/ConnectionLostMidQuery.hs @@ -0,0 +1,121 @@ +-- | Coverage for a connection reset while a query response is in flight - +-- after a valid handshake, not during one (contrast +-- 'Pqi.Conformance.Operation.Connectdb.HandshakeReset|). A hand-rolled +-- server completes the startup handshake normally (@AuthenticationOk@ then +-- @ReadyForQuery@), reads the client's @Query@ message, then resets the +-- connection - via @SO_LINGER@ with a zero timeout, so the close sends a +-- TCP @RST@ - instead of ever sending a result. +-- +-- Was found in @pqi-native@: none of 'Pqi.Native.Query.exec', +-- @execParams@, @prepare@, @execPrepared@, @describePrepared@, or +-- @describePortal@ wrapped their read loop in any exception handler at all - +-- unlike 'Pqi.Native.Connection.establish', which at least classifies a +-- broken read into a 'Pqi.errorMessage'. A connection lost mid-query used +-- to escape 'Pqi.exec' as a raw, uncaught 'System.IO.Error.IOException', +-- crashing the caller's thread outright rather than surfacing as any kind +-- of classified failure. libpq itself never throws here: @PQexec@ returns a +-- result with 'Pqi.FatalError' status and its own \"server closed the +-- connection unexpectedly\" message, the same wording it uses for a reset +-- during the handshake. +-- +-- Fixed by catching the read loop's 'System.IO.Error.IOException' in each of +-- those six flows and converting it into a classified 'Pqi.FatalError' +-- result via +-- 'Pqi.Native.Connection.connectionLostMessage'\/'Pqi.Native.Query.connectionLostResult', +-- the same classification 'HandshakeReset' exercises for the connect-time +-- case. +module Pqi.Conformance.Operation.Exec.ConnectionLostMidQuery + ( spec, + ) +where + +import Control.Concurrent (forkIO) +import Control.Exception (SomeException, bracket, try) +import qualified Data.ByteString as ByteString +import qualified Data.ByteString.Char8 as ByteString.Char8 +import qualified Network.Socket as Socket +import qualified Network.Socket.ByteString as Socket.ByteString +import qualified Pqi +import Pqi.Conformance.Prelude +import qualified Pqi.Conformance.Reference as Reference +import Test.Hspec + +spec :: Pqi.Adapter -> SpecWith ByteString +spec adapter = + describe "exec" do + describe "a connection reset while a query response is in flight" do + it "the candidate reports a classified failed result like the reference, instead of throwing" \_ -> + -- Both attempts share one listener (and so one port): the failure + -- message embeds the port number, and the candidate and reference + -- would otherwise always disagree on that one detail despite + -- matching in every way that matters. + withHandshakingThenResettingServer \port -> do + candidate <- attempt adapter port + reference <- attempt Reference.adapter port + candidate `shouldBe` reference + +-- | Connect, run one query against the given port (see +-- 'withHandshakingThenResettingServer'), and report the resulting result's +-- status and error message, or the exception's 'Show'n form if one escaped +-- 'Pqi.exec' - which is exactly what should never happen. +-- +-- @sslmode=disable@ keeps this comparable across adapters: libpq negotiates +-- SSL before the startup packet by default, so without it the exchange +-- would never reach the fake server's plain-protocol handshake, which +-- @pqi-native@ (which never attempts SSL) does not even send. The host is +-- given as a literal IP rather than a name so a failure message doesn't +-- gain a resolved-IP parenthetical the candidate would then also have to +-- reproduce. +attempt :: Pqi.Adapter -> Socket.PortNumber -> IO (Either String (Maybe (Pqi.ExecStatus, Maybe ByteString))) +attempt adapter port = do + let conninfo = + "host=127.0.0.1 port=" + <> ByteString.Char8.pack (show port) + <> " dbname=x user=x sslmode=disable" + bracket (Pqi.connectdb adapter conninfo) Pqi.finish \connection -> do + outcome <- try @SomeException (Pqi.exec connection "select 1") + case outcome of + Left err -> pure (Left (show err)) + Right mResult -> do + observed <- traverse (\r -> (,) <$> Pqi.resultStatus r <*> Pqi.resultErrorMessage r) mResult + pure (Right observed) + +-- | Bind a loopback listener on an ephemeral port and hand its port number +-- to the action, while a background thread serves every connection made to +-- it in turn: complete a minimal but valid startup handshake +-- (@AuthenticationOk@ then @ReadyForQuery@), read whatever the client sends +-- next (its @Query@ message), then reset the connection - via @SO_LINGER@ +-- with a zero timeout, which makes the kernel send a @RST@ instead of the +-- usual @FIN@ on close - rather than ever answering it. +withHandshakingThenResettingServer :: (Socket.PortNumber -> IO a) -> IO a +withHandshakingThenResettingServer action = + bracket open Socket.close \listener -> do + port <- Socket.socketPort listener + _ <- forkIO (try @SomeException (forever (serveOneQueryThenReset listener)) >> pure ()) + action port + where + open = do + address : _ <- + Socket.getAddrInfo + (Just Socket.defaultHints {Socket.addrSocketType = Socket.Stream}) + (Just "127.0.0.1") + (Just "0") + sock <- Socket.socket (Socket.addrFamily address) (Socket.addrSocketType address) (Socket.addrProtocol address) + Socket.bind sock (Socket.addrAddress address) + Socket.listen sock 8 + pure sock + +serveOneQueryThenReset :: Socket.Socket -> IO () +serveOneQueryThenReset listener = do + (conn, _) <- Socket.accept listener + _ <- Socket.ByteString.recv conn 4096 -- the startup packet + Socket.ByteString.sendAll conn authenticationOk + Socket.ByteString.sendAll conn readyForQuery + _ <- Socket.ByteString.recv conn 4096 -- the Query message + Socket.setSockOptValue conn Socket.Linger (Socket.SockOptValue (Socket.StructLinger 1 0)) + Socket.close conn + where + -- 'R', length 8 (self-inclusive), auth type 0 (Ok). + authenticationOk = ByteString.pack [0x52, 0x00, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00] + -- 'Z', length 5 (self-inclusive), status 'I' (idle). + readyForQuery = ByteString.pack [0x5A, 0x00, 0x00, 0x00, 0x05, 0x49]