Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix some FFI functions incorrectly being unsafe. #19

Closed
wants to merge 1 commit into from
Closed

Fix some FFI functions incorrectly being unsafe. #19

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 82 additions & 15 deletions src/Database/PostgreSQL/LibPQ.hsc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2373,6 +2373,21 @@ loUnlink connection oid
negError =<< c_lo_unlink c oid


-- Reminder on `unsafe` FFI:
phadej marked this conversation as resolved.
Show resolved Hide resolved
--
-- - `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)
Expand All @@ -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

Expand All @@ -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

Expand All @@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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"
Expand All @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it actually block? Aren't translations loaded once into memory?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they are loaded on demand:

https://github.com/postgres/postgres/blob/70f7da3e6ec865a120f3a3fed6173ce20f7d485a/src/interfaces/libpq/fe-misc.c#L1128-L1153

Here, bindtextdomain() sets the directory from which translation files are read, and dgettext() returns the translation string, so in between disk IO must be happening in practice.

Postgres puts an if (!already_bound) around it, but that is quite the implementation detail; to make PQescapeIdentifier() unsafe, we'd have to find some guarantee that it is imposible to call this function without some other safe function triggering the gettext code path first. This would be quite fragile.

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)

Expand Down