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.

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

waitToSetLock can block but is an unsafe FFI import #310

Open
hsyl20 opened this issue Dec 1, 2023 · 6 comments
Open

waitToSetLock can block but is an unsafe FFI import #310

hsyl20 opened this issue Dec 1, 2023 · 6 comments

Comments

@hsyl20
Copy link
Contributor

hsyl20 commented Dec 1, 2023

waitToSetLock uses c_fcntl_lock which is an unsafe FFI import, but the call may block indefinitely.

It was the root cause of https://gitlab.haskell.org/ghc/ghc/-/issues/15485 in GHC <= 9.0

This is related to #34 but this particular call should be uncontroversial to fix (I think).

@hasufell
Copy link
Member

hasufell commented Dec 2, 2023

@hsyl20 I'm not entirely sure what you're proposing. Can you open a PR?

@hsyl20
Copy link
Contributor Author

hsyl20 commented Dec 4, 2023

c_fcntl_lock is defined like this in base:System.Posix.Internals (for the non JS variant):

foreign import capi unsafe "HsBase.h fcntl"
   c_fcntl_lock  :: CInt -> CInt -> Ptr CFLock -> IO CInt

It's an unsafe foreign import; we shouldn't use it with FSETLKW which blocks indefinitely. We should define and use a safe foreign import instead (e.g. c_fcntl_lock_safe).

I don't know why c_fcntl_lock is exposed from base but it makes opening a PR more difficult. Either we add the safe foreign import to base but it probably require a CLC proposal, or we add it to unix but then it's weird to have one part here and the other in base, or we move everything to unix but it probably requires a CLC proposal and some deprecation period too.

@hasufell
Copy link
Member

hasufell commented Dec 4, 2023

Any opinions @Bodigrim @hs-viktor ?

@hsyl20
Copy link
Contributor Author

hsyl20 commented Dec 4, 2023

Another alternative: add a safety parameter to c_fcntl_lock in base to force the caller to explicitly select between the safe/unsafe versions of the call to use. We'd just need some CPP in unix to adapt to the new API.

@vdukhovni
Copy link

We should stop using the import from System.Posix.Internals and use a private safe foreign import.

@vdukhovni
Copy link

No CLC proposal required, a private foreign import in System/Posix/IO/Common.hsc does not change any APIs. What the CLC decides to do with the existing public API in System.Posix.Internals is a separate issue, not material to unix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants