Fix memory leak in getaddrinfo and make it async exception safe#591
Fix memory leak in getaddrinfo and make it async exception safe#591arybczak wants to merge 1 commit intohaskell:masterfrom
Conversation
haskell#587 removed the call to `c_freeaddrinfo` in `getaddrinfo`. This restores it and in addition makes the function async exception safe.
kazu-yamamoto
left a comment
There was a problem hiding this comment.
Thank you for bringing this issue.
This is a nice catch.
This is a common pattern.
So, I would like you to check if bracketOnError can be used instead of mask.
|
|
|
I meant |
|
See the following definition. It uses the same pattern as you wrote. bracketOnError
:: IO a -- ^ computation to run first (\"acquire resource\")
-> (a -> IO b) -- ^ computation to run last (\"release resource\")
-> (a -> IO c) -- ^ computation to run in-between
-> IO c -- returns the value from the in-between computation
bracketOnError before after thing =
mask $ \restore -> do
a <- before
restore (thing a) `onException` after a |
|
@kazu-yamamoto seems to me that the result of |
Not really, because I used In any case, I tried to use bracket
(do ret <- c_getaddrinfo c_node c_service c_hints ptr_ptr_addrs
ptr_addrs <- if ret == 0
then peek ptr_ptr_addrs
else return nullPtr
return (ret, ptr_addrs)
)
(\(ret, ptr_addrs) -> do
-- Don't call freeaddrinfo with NULL, it segfaults on some
-- platforms.
when (ret == 0) $ c_freeaddrinfo ptr_addrs
)
(\(ret, ptr_addrs) ->
if ret == 0
then followAddrInfo ptr_addrs
else do
err <- gai_strerror ret
ioError $ ioeSetErrorString
(mkIOError NoSuchThing message Nothing Nothing)
err
)that's why I unrolled it in the first place and I don't think trying to force it is worth it. If you disagree, please fix it the way you like. Also, please make a new release with this fix asap and deprecate 3.2.3.0, 3.2.4.0, 3.2.5.0 and 3.2.6.0 on Hackage because this is a very serious bug. |
|
How about something like this? getAddrs = do
ret <- c_getaddrinfo c_node c_service c_hints
if ret == 0
then Right <$> peek ptr_ptr_addrs
else pure $ Left ret
addrErr ret = do
err <- gai_strerror ret
ioError $ ioeSetErrorString
(mkIOError NoSuchThing message Nothing Nothing)
err
bracket
getAddrs
(mapM_ c_freeaddrinfo)
(either addrErr followAddrInfo) |
You are right. After this discussion, the original PR code is simplest, I think. |
|
v3.2.7.0 has been released. |
|
@kazu-yamamoto |
|
My bad. X-( |
## Version 3.2.7.0 * Using nested `bracket` for `gracefulClose`. [#591](haskell/network#590) * Fix memory leak in getaddrinfo and make it async exception safe. [#591](haskell/network#591) * Make call to c_free async exception safe. [#592](haskell/network#592) ## Version 3.2.6.0 * fixing the Show instance of IPv4-mapped IPv6 address on little endian machines ## Version 3.2.5.0 * `gracefulClose` based on STM racing and `timeout`. [#587](haskell/network#587) ## Version 3.2.4.0 * New API: setSockOptValue. [#588](haskell/network#588) ## Version 3.2.3.0 * Making getAddrInfo polymorphic [#587](haskell/network#587) ## Version 3.2.2.0 * New API: waitReadSocketSTM, waitAndCancelReadSocketSTM, waitWriteSocketSTM, waitAndCancelWriteSocketSTM [#586](haskell/network#586) * Checking the length of ASCII string allowing trailing 0. [#585](haskell/network#585) ## Version 3.2.1.0 * Trying to release with the latest autoreconf. Packing "network" in the local directory instead of CI. * Remove includes from .cabal-file [#583](haskell/network#583) * making gracefulClose more graceful [#580](haskell/network#580) * Update config.guess, config.sub to their latest versions [#579](haskell/network#579)
#587 removed the call to
c_freeaddrinfoingetaddrinfo.This restores it and in addition makes the function async exception safe.