From a16375f7c2654aa2ecb5be4ffdcf66a43e6d1d92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niklas=20Hamb=C3=BCchen?= Date: Tue, 19 Apr 2022 23:19:48 +0200 Subject: [PATCH] Fix some FFI functions incorrectly being `unsafe`. Those functions can call back or or do blocking IO, which is not allowed for `unsafe` calls. This fixes a case I encountered on our CI machine where `unsafe` on `PQisBusy()` resulted in GHC RTS hangs when a postgresql notice processor was set to call back into Haskell. --- src/Database/PostgreSQL/LibPQ.hsc | 97 ++++++++++++++++++++++++++----- 1 file changed, 82 insertions(+), 15 deletions(-) diff --git a/src/Database/PostgreSQL/LibPQ.hsc b/src/Database/PostgreSQL/LibPQ.hsc index 8496879..4982e9b 100644 --- a/src/Database/PostgreSQL/LibPQ.hsc +++ b/src/Database/PostgreSQL/LibPQ.hsc @@ -2373,6 +2373,21 @@ loUnlink connection oid negError =<< c_lo_unlink c oid +-- Reminder on `unsafe` FFI: +-- +-- - `unsafe` calls MUST NOT do any blocking IO! +-- - `unsafe` calls MUST NOT call back into Haskell! +-- Otherwise the program may hang permanently. +-- +-- Here is an example of the seemingly innocent function `PQisBusy()` +-- actually calling back into Haskell in some cases: +-- `PQisBusy()` in some cases calls `pqParseInput3()` +-- which calls `pqGetErrorNotice3()` which can call back into a +-- user-defined error notice processing callback if set, +-- which the user might have set to be a Haskell function. +-- +-- See https://downloads.haskell.org/ghc/9.2.2/docs/html/users_guide/exts/ffi.html#foreign-imports-and-multi-threading +-- for details. foreign import ccall "libpq-fe.h PQconnectdb" c_PQconnectdb :: CString ->IO (Ptr PGconn) @@ -2383,51 +2398,66 @@ foreign import ccall "libpq-fe.h PQconnectStart" foreign import ccall "libpq-fe.h PQconnectPoll" c_PQconnectPoll :: Ptr PGconn ->IO CInt +-- | `unsafe` is OK because `PQdb` calls no functions. foreign import ccall unsafe "libpq-fe.h PQdb" c_PQdb :: Ptr PGconn -> IO CString +-- | `unsafe` is OK because `PQuser` calls no functions. foreign import ccall unsafe "libpq-fe.h PQuser" c_PQuser :: Ptr PGconn -> IO CString +-- | `unsafe` is OK because `PQpass` calls no functions. foreign import ccall unsafe "libpq-fe.h PQpass" c_PQpass :: Ptr PGconn -> IO CString +-- | `unsafe` is OK because `PQhost` calls no functions. foreign import ccall unsafe "libpq-fe.h PQhost" c_PQhost :: Ptr PGconn -> IO CString +-- | `unsafe` is OK because `PQport` calls no functions. foreign import ccall unsafe "libpq-fe.h PQport" c_PQport :: Ptr PGconn -> IO CString +-- | `unsafe` is OK because `PQport` calls no functions. foreign import ccall unsafe "libpq-fe.h PQoptions" c_PQoptions :: Ptr PGconn -> IO CString +-- | `unsafe` is OK because `PQbackendPID` calls no functions. foreign import ccall unsafe "libpq-fe.h PQbackendPID" c_PQbackendPID :: Ptr PGconn -> IO CInt +-- | `unsafe` is OK because `PQconnectionNeedsPassword` calls no functions +-- (it calls `PQpass`, which does the same). foreign import ccall unsafe "libpq-fe.h PQconnectionNeedsPassword" c_PQconnectionNeedsPassword :: Ptr PGconn -> IO CInt +-- | `unsafe` is OK because `PQconnectionUsedPassword` calls no functions. foreign import ccall unsafe "libpq-fe.h PQconnectionUsedPassword" c_PQconnectionUsedPassword :: Ptr PGconn -> IO CInt +-- | `unsafe` is OK because `PQstatus` calls no functions. foreign import ccall unsafe "libpq-fe.h PQstatus" c_PQstatus :: Ptr PGconn -> IO CInt +-- | `unsafe` is OK because `PQtransactionStatus` calls no functions. foreign import ccall unsafe "libpq-fe.h PQtransactionStatus" c_PQtransactionStatus :: Ptr PGconn -> IO CInt foreign import ccall "libpq-fe.h PQparameterStatus" c_PQparameterStatus :: Ptr PGconn -> CString -> IO CString +-- | `unsafe` is OK because `PQprotocolVersion` calls no functions. foreign import ccall unsafe "libpq-fe.h PQprotocolVersion" c_PQprotocolVersion :: Ptr PGconn -> IO CInt +-- | `unsafe` is OK because `PQserverVersion` calls no functions. foreign import ccall unsafe "libpq-fe.h PQserverVersion" c_PQserverVersion :: Ptr PGconn -> IO CInt foreign import ccall "dynamic" mkLibpqVersion :: FunPtr Int -> Int +-- | `unsafe` is OK because `PQsocket` calls no functions. foreign import ccall unsafe "libpq-fe.h PQsocket" c_PQsocket :: Ptr PGconn -> IO CInt @@ -2451,6 +2481,7 @@ foreign import ccall "libpq-fe.h PQresetStart" foreign import ccall "libpq-fe.h PQresetPoll" c_PQresetPoll :: Ptr PGconn ->IO CInt +-- | `unsafe` is OK because `PQclientEncoding` calls no functions. foreign import ccall unsafe "libpq-fe.h PQclientEncoding" c_PQclientEncoding :: Ptr PGconn -> IO CInt @@ -2461,6 +2492,7 @@ foreign import ccall "libpq-fe.h PQsetClientEncoding" c_PQsetClientEncoding :: Ptr PGconn -> CString -> IO CInt type PGVerbosity = CInt +-- | `unsafe` is OK because `PQclientEncoding` calls no functions. foreign import ccall unsafe "libpq-fe.h PQsetErrorVerbosity" c_PQsetErrorVerbosity :: Ptr PGconn -> PGVerbosity -> IO PGVerbosity @@ -2507,21 +2539,23 @@ foreign import ccall "libpq-fe.h &PQfreeCancel" foreign import ccall "libpq-fe.h PQcancel" c_PQcancel :: Ptr PGcancel -> CString -> CInt -> IO CInt -foreign import ccall unsafe "libpq-fe.h PQnotifies" +foreign import ccall "libpq-fe.h PQnotifies" c_PQnotifies :: Ptr PGconn -> IO (Ptr Notify) foreign import ccall "libpq-fe.h PQconsumeInput" c_PQconsumeInput :: Ptr PGconn -> IO CInt -foreign import ccall unsafe "libpq-fe.h PQisBusy" +foreign import ccall "libpq-fe.h PQisBusy" c_PQisBusy :: Ptr PGconn -> IO CInt foreign import ccall "libpq-fe.h PQsetnonblocking" c_PQsetnonblocking :: Ptr PGconn -> CInt -> IO CInt +-- | `unsafe` is OK because `PQisnonblocking` calls only a macro that calls no functions. foreign import ccall unsafe "libpq-fe.h PQisnonblocking" c_PQisnonblocking :: Ptr PGconn -> IO CInt +-- | `unsafe` is OK because `PQsetSingleRowMode` calls no functions. foreign import ccall unsafe "libpq-fe.h PQsetSingleRowMode" c_PQsetSingleRowMode :: Ptr PGconn -> IO CInt @@ -2553,67 +2587,91 @@ foreign import ccall "libpq-fe.h PQdescribePortal" foreign import ccall "libpq-fe.h &PQclear" p_PQclear :: FunPtr (Ptr PGresult ->IO ()) +-- | `unsafe` is OK because `PQresultStatus` calls no functions. foreign import ccall unsafe "libpq-fe.h PQresultStatus" c_PQresultStatus :: Ptr PGresult -> IO CInt -foreign import ccall unsafe "libpq-fe.h PQresStatus" +-- Must not be `unsafe` because `PQresStatus` may use `libpq_gettext()` that may do IO. +foreign import ccall "libpq-fe.h PQresStatus" c_PQresStatus :: CInt -> IO CString +-- | `unsafe` is OK because `PQresultStatus` calls no functions. foreign import ccall unsafe "libpq-fe.h PQresultErrorMessage" c_PQresultErrorMessage :: Ptr PGresult -> IO CString foreign import ccall "libpq-fe.h PQresultErrorField" c_PQresultErrorField :: Ptr PGresult -> CInt -> IO CString +-- | `unsafe` is OK because `PQntuples` calls no functions. foreign import ccall unsafe "libpq-fe.h PQntuples" c_PQntuples :: Ptr PGresult -> CInt +-- | `unsafe` is OK because `PQnfields` calls no functions. foreign import ccall unsafe "libpq-fe.h PQnfields" c_PQnfields :: Ptr PGresult -> CInt -foreign import ccall unsafe "libpq-fe.h PQfname" +-- Must not be `unsafe` because `PQfname` may call `check_field_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQfname" c_PQfname :: Ptr PGresult -> CInt -> IO CString +-- | `unsafe` is OK because `PQfnumber` calls only `pg_tolower()`, which calls +-- only C stdlib functions that certainly will not call back into Haskell. +-- Note `pg_tolower()` also calls `strdup()` and `free()`, so if one of those +-- was overridden with e.g. glibc hooks to call back into Haskell, this would be wrong. +-- However, these functions are generally considered to not call back into Haskell. foreign import ccall unsafe "libpq-fe.h PQfnumber" c_PQfnumber :: Ptr PGresult -> CString -> IO CInt -foreign import ccall unsafe "libpq-fe.h PQftable" +-- Must not be `unsafe` because `PQftable` may call `check_field_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQftable" c_PQftable :: Ptr PGresult -> CInt -> IO Oid -foreign import ccall unsafe "libpq-fe.h PQftablecol" +-- Must not be `unsafe` because `PQftablecol` may call `check_field_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQftablecol" c_PQftablecol :: Ptr PGresult -> CInt -> IO CInt -foreign import ccall unsafe "libpq-fe.h PQfformat" +-- Must not be `unsafe` because `PQfformat` may call `check_field_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQfformat" c_PQfformat :: Ptr PGresult -> CInt -> IO CInt +-- Must not be `unsafe` because `PQftype` may call `check_field_number()` which may call `pqInternalNotice()`. foreign import ccall unsafe "libpq-fe.h PQftype" c_PQftype :: Ptr PGresult -> CInt -> IO Oid -foreign import ccall unsafe "libpq-fe.h PQfmod" +-- Must not be `unsafe` because `PQfmod` may call `check_field_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQfmod" c_PQfmod :: Ptr PGresult -> CInt -> IO CInt -foreign import ccall unsafe "libpq-fe.h PQfsize" +-- Must not be `unsafe` because `PQfsize` may call `check_field_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQfsize" c_PQfsize :: Ptr PGresult -> CInt -> IO CInt -foreign import ccall unsafe "libpq-fe.h PQgetvalue" +-- Must not be `unsafe` because `PQgetvalue` may call `check_tuple_field_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQgetvalue" c_PQgetvalue :: Ptr PGresult -> CInt -> CInt -> IO CString -foreign import ccall unsafe "libpq-fe.h PQgetisnull" +-- Must not be `unsafe` because `PQgetisnull` may call `check_tuple_field_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQgetisnull" c_PQgetisnull :: Ptr PGresult -> CInt -> CInt -> IO CInt -foreign import ccall unsafe "libpq-fe.h PQgetlength" +-- Must not be `unsafe` because `PQgetlength` may call `check_tuple_field_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQgetlength" c_PQgetlength :: Ptr PGresult -> CInt -> CInt -> IO CInt +-- | `unsafe` is OK because `PQnparams` calls no functions. foreign import ccall unsafe "libpq-fe.h PQnparams" c_PQnparams :: Ptr PGresult -> IO CInt -foreign import ccall unsafe "libpq-fe.h PQparamtype" +-- Must not be `unsafe` because `PQparamtype` may call `check_param_number()` which may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQparamtype" c_PQparamtype :: Ptr PGresult -> CInt -> IO Oid +-- | `unsafe` is OK because `PQcmdStatus` calls no functions. foreign import ccall unsafe "libpq-fe.h PQcmdStatus" c_PQcmdStatus :: Ptr PGresult -> IO CString -foreign import ccall unsafe "libpq-fe.h PQcmdTuples" +-- Must not be `unsafe` because `PQcmdTuples` may call `pqInternalNotice()`. +foreign import ccall "libpq-fe.h PQcmdTuples" c_PQcmdTuples :: Ptr PGresult -> IO CString foreign import ccall "libpq-fe.h PQescapeStringConn" @@ -2636,33 +2694,42 @@ foreign import ccall "libpq-fe.h PQunescapeBytea" -> Ptr CSize -> IO (Ptr Word8) -- Actually (IO (Ptr CUChar)) -foreign import ccall unsafe "libpq-fe.h PQescapeIdentifier" +-- Must not be `unsafe` because `PQescapeIdentifier` may call `libpq_gettext()` that may do IO. +foreign import ccall "libpq-fe.h PQescapeIdentifier" c_PQescapeIdentifier :: Ptr PGconn -> CString -> CSize -> IO CString +-- | `unsafe` is OK because `PQfreemem` only calls `free()`. foreign import ccall unsafe "libpq-fe.h &PQfreemem" p_PQfreemem :: FunPtr (Ptr a -> IO ()) +-- | `unsafe` is OK because `PQfreemem` only calls `free()`. foreign import ccall unsafe "libpq-fe.h PQfreemem" c_PQfreemem :: Ptr a -> IO () +-- | `unsafe` is OK because `hs_postgresql_libpq_malloc_noticebuffer` only calls `malloc()`. foreign import ccall unsafe "noticehandlers.h hs_postgresql_libpq_malloc_noticebuffer" c_malloc_noticebuffer :: IO (Ptr CNoticeBuffer) +-- | `unsafe` is OK because `hs_postgresql_libpq_malloc_noticebuffer` only calls `free()`. foreign import ccall unsafe "noticehandlers.h hs_postgresql_libpq_free_noticebuffer" c_free_noticebuffer :: Ptr CNoticeBuffer -> IO () +-- | `unsafe` is OK because `hs_postgresql_libpq_get_notice` calls no functions. foreign import ccall unsafe "noticehandlers.h hs_postgresql_libpq_get_notice" c_get_notice :: Ptr CNoticeBuffer -> IO (Ptr PGnotice) +-- | `unsafe` is OK because `hs_postgresql_libpq_discard_notices` calls no functions. foreign import ccall unsafe "noticehandlers.h &hs_postgresql_libpq_discard_notices" p_discard_notices :: FunPtr NoticeReceiver +-- | `unsafe` is OK because `hs_postgresql_libpq_store_notices` calls only `PQresultErrorMessage`, which calls no functions. foreign import ccall unsafe "noticehandlers.h &hs_postgresql_libpq_store_notices" p_store_notices :: FunPtr NoticeReceiver +-- | `unsafe` is OK because `PQsetNoticeReceiver` calls no functions. foreign import ccall unsafe "libpq-fe.h PQsetNoticeReceiver" c_PQsetNoticeReceiver :: Ptr PGconn -> FunPtr NoticeReceiver -> Ptr CNoticeBuffer -> IO (FunPtr NoticeReceiver)