Bind `fdopendir` and `openat` #110

strake · 2018-05-08T04:53:54Z

No description provided.

strake · 2018-05-21T20:17:08Z

ping

hvr · 2018-05-21T20:54:20Z

Sorry haven't yet had time to review this. I'm a bit worried about two things here: a) routing everything through openat(2) and thus replace open(2) and b) these are POSIX.1-2008 entry points

Are we sure all non-EOL'ed target platforms provide/support openat(2) and fdopendir(3)?

strake · 2018-05-21T21:12:25Z

The package description specifies POSIX.1-2008.

Is somewhere a list of supported platforms of this package? I checked all supported Unix platforms of GHC and they all have both openat and fdopendir — indeed, both GNU libc on Linux and FreeBSD libc now call openat rather than open (not sure about OS X).

hvr · 2018-05-21T21:43:10Z

I only mentioned POSIX.1-2008 because this means these could be recent enough that some less mainstream platforms we support may not have supported them yet. And if that's the case, we'd need to consider conditional compilation & autoconf tests.

Unfortunately I don't have a comprehensive list of supported platforms but we also support e.g. AIX, Solaris, OpenBSD and Android

strake · 2018-05-22T00:04:32Z

PTAL

strake · 2018-07-03T22:05:21Z

ping

strake · 2018-08-19T06:36:13Z

ping

strake · 2019-11-23T20:43:21Z

@hvr ping

hasufell

@hvr what is blocking this? This is required to fix dangerous bugs in the directory package: haskell/directory#97 and #134

hasufell · 2020-01-05T13:23:46Z

System/Posix/Directory/Common.hsc

 module System.Posix.Directory.Common (
       DirStream(..), CDir, CDirent, DirStreamOffset(..),
+#ifdef HAVE_FDOPENDIR
+       openDirStreamFd,


IMO, this should be in its own module System.Posix.Directory.Fd, no?

hasufell · 2020-01-05T13:23:59Z

System/Posix/Directory/Common.hsc

 data {-# CTYPE "struct dirent" #-} CDirent

+#ifdef HAVE_FDOPENDIR
+-- | @openDirStreamFd fd@ calls @fdopendir@ to obtain a


This should document that you must not close the file descriptor afterwards with a link to the POSIX documentation and that the file descriptor is closed when the stream is closed.

hasufell · 2020-01-31T20:02:19Z

@Rufflewind @hvr ping

hasufell · 2020-02-11T23:37:13Z

@hvr ping

cartazio · 2020-02-12T21:12:42Z

@chessai @hvr @Bodigrim This seems like a pretty decent patch, what are the support surface area concerns blocking it at the moment?

like, what OS's does the haskell Unix need to support, and whats missing to make this or something similar a good patch?

hasufell · 2020-02-16T09:35:49Z

Guys, why is no one responding here? It's discouraging for contributors to wait 2 years for actionable input.

Bodigrim · 2020-02-17T18:36:12Z

System/Posix/IO.hsc

    OpenMode(..),
    OpenFileFlags(..), defaultFileFlags,
-    openFd, createFile,
+    openFd, openFdAt, createFile, createFileAt,


Should not openFdAt and createFileAt be guarded by #ifdef HAVE_OPENAT?

Bodigrim · 2020-02-17T18:36:51Z

System/Posix/IO.hsc

+       -> IO Fd
+openFd = openFdAt Nothing
+
+openFdAt :: Maybe Fd


It would be nice to add a haddock comment for openFdAt.

Bodigrim · 2020-02-17T18:37:04Z

System/Posix/IO.hsc

+createFile :: FilePath -> FileMode -> IO Fd
+createFile = createFileAt Nothing
+
+createFileAt :: Maybe Fd -> FilePath -> FileMode -> IO Fd


It would be nice to add a haddock comment for createFileAt.

Bodigrim · 2020-02-17T18:37:27Z

System/Posix/IO/ByteString.hsc

+       -> IO Fd
+openFd = openFdAt Nothing
+
+openFdAt :: Maybe Fd


It would be nice to add a haddock comment for openFdAt.

Bodigrim · 2020-02-17T18:37:39Z

System/Posix/IO/ByteString.hsc

+createFile :: RawFilePath -> FileMode -> IO Fd
+createFile = createFileAt Nothing
+
+createFileAt :: Maybe Fd -> RawFilePath -> FileMode -> IO Fd


It would be nice to add a haddock comment for createFileAt.

Bodigrim · 2020-02-17T18:38:43Z

System/Posix/IO/ByteString.hsc

    OpenMode(..),
    OpenFileFlags(..), defaultFileFlags,
-    openFd, createFile,
+    openFd, openFdAt, createFile, createFileAt,


Should not openFdAt and createFileAt be guarded by #ifdef HAVE_OPENAT?

cartazio · 2020-02-17T18:54:32Z

thank you for getting this rolling @Bodigrim

hasufell · 2020-04-14T11:25:31Z

Please double check this code on mac!

I have a similar code: https://github.com/hasufell/hpath/blob/06b5a46cf81371204870ad3ebf78af2e59fbbdd7/hpath-posix/src/System/Posix/RawFilePath/Directory/Traversals.hs#L179

And fdopendir usage here causes d_name to have the first char truncated on mac only:

https://travis-ci.org/github/hasufell/hpath/jobs/674795844#L1167-L1168

I'm not sure why. The use of unsafeCoerce seems safe (just converting to newtype).

EDIT: @cartazio debugged this and it appears there are two possible functions for fdopendir https://opensource.apple.com/source/Libc/Libc-1244.1.7/include/dirent.h.auto.html

One of them crashes hard and the above code will trigger it. The fix was to use:

foreign import capi unsafe "dirent.h fdopendir"
    c_fdopendir :: Posix.Fd -> IO (Ptr CDir)

cartazio · 2020-04-14T22:42:45Z

emphasizing what @hasufell says,

at least on OSX, a number of newer unix calls really need to be bound via capi (an implicit header aware c wrapper) or an explicit c call with the correct system headers included

reproducing the example also mentioned above that i cooked up and shared with julian:

foreign import capi unsafe "dirent.h fdopendir"
    c_fdopendir :: Posix.Fd -> IO (Ptr CDir)

the header part of the capi syntax is crucial for this to compile and link correctly! (its actually impossible to bind the correct underlying system call symbol directly)

l29ah · 2020-08-31T20:56:18Z

wtf just merge it already

hasufell · 2020-08-31T21:14:48Z

wtf just merge it already

The code is broken. See above.

l29ah · 2020-08-31T22:42:36Z

The code works on linux and doesn't break anything. If it's bugged on some other platform, it could be addressed separately as a bugfix. Meanwhile, MacOS doesn't even strive to be POSIX these days it seems: https://www.quora.com/Is-Mac-OS-X-more-POSIX-compliant-than-Linux/answer/Madars-Oz

cartazio · 2020-08-31T22:58:22Z

Julian and I figured out how to correctly bind some of this stuff on OS X. But incorrectly binding it will make programs flat out crash and silently corrupt data.

A pr that does the capi indirection would be safe to merge. But this pr is flat out wrong. Someone prepare a pr that does the capi stuff and then we can make it work

cartazio · 2020-09-01T01:27:53Z

Cc @Bodigrim and victor

Bodigrim · 2020-11-14T18:25:34Z

@l29ah currently the only OS forbidden to use unix package is Windows:

unix/unix.cabal

Lines 67 to 71 in f7b956c

    
           if os(windows) 
        
               -- This package currently supports neither Cygwin nor MinGW, 
        
               -- therefore os(windows) is effectively not supported. 
        
               build-depends: unbuildable<0 
        
               buildable: False

So even if MacOS is not POSIX compliant, we cannot just silently break it.

l29ah · 2020-11-15T01:13:33Z

Also the function definition can be put under a conditional if you don't want to create a false hope for Mac users.

cartazio · 2020-11-15T01:32:58Z

Bodigrim: it looks like the binding is capi based now. @hasufell what was the min version of mac we found that supported this call?

Bodigrim · 2020-11-15T01:45:37Z

@l29ah Providing conditional API is an option, but so far unix avoided it, sticking to a role of a common denominator for all non-Windows platforms. One could position unix as a wrapper for POSIX - and woe to systems which are not, but I'd say that such approach would greatly diminish its utility.

hs-viktor · 2021-02-15T05:39:03Z

I've rebased this PR on master and pushed two fixup commits to address the CApiFFI issue for MacOS and missing #if guards for openat functions.

All CI targets now pass. It just remains to do a final closer review, but we should be able to merge this (perhaps squashed) soon barring unexpected issues.

hs-viktor

This is almost done, but I am concerned about the conditionally defined functions. I don't see how a dependent application or library can safely make use of these functions, in the sense that it will at least reliably compile on all platforms. Do they need to also run configure and use HAVE_OPENAT, ...?

There should likely be some way to discover whether these are available at runtime. So some sort of stub implementation needs to exist, and some way to test whether the function is expected to work, or whether the caller should take an alternative code path (or give up).

Perhaps just simple boolean predicates that are exported along with these?

Unless we've reached the point where the only supported Posix platforms all have openat, et. al. and we just fail to build unix otherwise. So that these are then always available.

hs-viktor · 2021-02-15T05:47:09Z

System/Posix/Directory/Common.hsc

+#ifdef HAVE_FDOPENDIR
+-- | @unsafeOpenDirStreamFd fd@ calls @fdopendir@ to obtain a directory stream
+--   for @fd@. @fd@ must not be otherwise used after this; see
+--   <https://pubs.opengroup.org/onlinepubs/9699919799/functions/fdopendir.html POSIX>.


Perhaps worth mentioning that if an exception is thrown here, closing the file descriptor remains the responsibility of the caller, who may need to use something like bracketOnError to handle this possibility.

Alternatively, perhaps this function should arrange to close the descriptor if c_fdopendir fails. This way the descriptor is never leaked. I'm inclined to recommend this approach.

@Bodigrim I see you've approved the MR, but perhaps you did not notice the above issue/question amidst the flurry of activity to resolve the easier obstacles. I do think we still need to do something here...

hs-viktor · 2021-02-15T06:37:23Z

There should likely be some way to discover whether these are available at runtime. So some sort of stub implementation needs to exist, and some way to test whether the function is expected to work, or whether the caller should take an alternative code path (or give up).

Perhaps just simple boolean predicates that are exported along with these?

One simple way to handle these is to export a Maybe T where T is the type of the optionally implemented function, and not export the function itself. A caller can then write:

 case getOpenFdAt of
  Just f -> f ...
  Nothing -> ...

which, with appropriate inlining, will optimise out the test and take just the appropriate branch.

System/Posix/Directory/Common.hsc

hs-viktor · 2021-02-15T20:00:49Z

The openat function was added in:

Linux 2.6.16 with glibc 2.4 (Mar 2006)
FreeBSD 8.0 (Nov 2009)
OpenBSD 5.0 (Nov 2011)
MacOS 10.10 (Oct 2014)
NetBSD 7.0 (Aug 2015)

Has it been established long enough that we can just require it unconditionally?

hs-viktor · 2021-02-15T20:31:50Z

The timeline for fdopendir seems to be:

glibc 2.4
FreeBSD 8.0
OpenBSD 5.0
MacOS 10.10
NetBSD 6.0

Pretty much the same, but slightly older with NetBSD. So if we can require openat outright, we could do the same with fdopendir and dispense with the CPP conditions.

Bodigrim · 2021-02-15T22:05:37Z

Has it been established long enough that we can just require it unconditionally?

Yes, I believe 5+ years are enough. It is better to enable it unconditionally now than have a breaking switch from Maybe T to T in future, or pay for Maybe infinitely long.

cartazio · 2021-02-15T22:24:36Z

Sounds good to me

…

On Mon, Feb 15, 2021 at 5:05 PM Bodigrim ***@***.***> wrote: Has it been established long enough that we can just require it unconditionally? Yes, I believe 5+ years are enough. It is better to enable it unconditionally now than have a breaking switch from Maybe T to T in future, or pay for Maybe infinitely long. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#110 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAABBQTHZFYQYHQJE24MGKTS7GLEHANCNFSM4E6YBVTQ> .

hs-viktor · 2021-02-15T23:50:03Z

The latest commit drops the #if guards. Builds on all the platforms on which run CI. I also added missing haddocks. Please review.

Separately from this MR, I really dislike the way that the file open flags are handled. We only support a fixed subset, hard-code as a list of booleans in a structure. This is IMHO awful. It screams out for pattern synonyms and a single numeric flag field, that users can set to values that are not yet known to the library, if they know what they're doing.

I should open a separate issue...

hs-viktor

Disposition of file descriptor passed to fdopendir when the call fails remains as yet unaddressed. Should it be automatically closed (I think so), but otherwise callers need to know that they have to catch the exception and do something with the descriptor.

Bodigrim · 2021-02-16T00:37:50Z

I'm in favor of closing it automatically.

vdukhovni · 2021-02-16T01:28:40Z

I'm in favor of closing it automatically.

OK, I'll arrange to close it with c_close, but it is perhaps worth noting that if any of the descriptors being passed into the library are being used in some other thread with threadWaitRead or threadWaitWrite, then they have to be closed with closeFdWith, rather than just c_close, so that the IO Manager can do appropriate cleanup.

Nothing in unix uses the IO Manager interfaces, but we perhaps need to make it clear to users that they need to be careful with passing such descriptors into unix calls that might close the descriptor. Not likely to be an issue with a directory handle, so I think it is fairly safe to not worry too much with fdopendir, but something that may warrant a closer look elsewhere...

This way the caller never ends up owning the fd after the call. Otherwise, cleanup of either the DirStream or else the file-descriptor would be difficult to implement correctly.

hs-viktor · 2021-02-16T07:48:06Z

Final cut:

unsafeOpenDirStreamFd input descriptor never left unpainted
squashed
changelog updated

Looking at the surrounding code makes me wish for a time-machine to address other issues that will require some care, but not in this PR. :-(

Issues addressed.

Bodigrim

Looks good!

strake force-pushed the at branch 4 times, most recently from a6894f6 to a570f74 Compare May 8, 2018 23:44

hvr added this to the 2.8.0.0 milestone Aug 19, 2018

hasufell reviewed Jan 5, 2020

View reviewed changes

hasufell mentioned this pull request Jan 5, 2020

way to prevent openDirStream from following symlinks #134

Closed

strake force-pushed the at branch from cd5f007 to 59670ea Compare January 14, 2020 02:41

Bodigrim reviewed Feb 17, 2020

View reviewed changes

hs-viktor force-pushed the at branch from 59670ea to c699c44 Compare February 15, 2021 03:58

hs-viktor reviewed Feb 15, 2021

View reviewed changes

hasufell reviewed Feb 15, 2021

View reviewed changes

System/Posix/Directory/Common.hsc Show resolved Hide resolved

Bodigrim approved these changes Feb 16, 2021

View reviewed changes

hs-viktor previously requested changes Feb 16, 2021

View reviewed changes

strake and others added 3 commits February 16, 2021 02:39

Add openDirStreamFd, openFileAt and createFileAt

970c461

On error close fd passed to unsafeOpenDirStreamFd.

697b1b1

This way the caller never ends up owning the fd after the call. Otherwise, cleanup of either the DirStream or else the file-descriptor would be difficult to implement correctly.

Update changelog

0d89238

hs-viktor force-pushed the at branch from 090b16e to 0d89238 Compare February 16, 2021 07:44

hs-viktor requested a review from Bodigrim February 16, 2021 07:49

Bodigrim approved these changes Feb 16, 2021

View reviewed changes

hs-viktor merged commit 9726727 into haskell:master Feb 16, 2021

Bodigrim linked an issue Apr 11, 2022 that may be closed by this pull request

way to prevent openDirStream from following symlinks #134

Closed

This was referenced Jul 17, 2022

Using on MacOS / iOS - semTrywait: invalid argument (Bad file descriptor) #218

Closed

Fix 'semTrywait: invalid argument (Bad file descriptor)' wrt #218 #229

Merged

hasufell mentioned this pull request Sep 19, 2025

Remove readDirStreamWithPtr; replace readdir_r() with readdir() #349

Open

Bind fdopendir and openat #110

Bind fdopendir and openat #110

Uh oh!

Conversation

strake commented May 8, 2018

Uh oh!

strake commented May 21, 2018

Uh oh!

hvr commented May 21, 2018

Uh oh!

strake commented May 21, 2018

Uh oh!

hvr commented May 21, 2018

Uh oh!

strake commented May 22, 2018

Uh oh!

strake commented Jul 3, 2018

Uh oh!

strake commented Aug 19, 2018

Uh oh!

strake commented Nov 23, 2019

Uh oh!

hasufell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasufell Jan 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasufell commented Jan 31, 2020

Uh oh!

hasufell commented Feb 11, 2020

Uh oh!

cartazio commented Feb 12, 2020

Uh oh!

hasufell commented Feb 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cartazio commented Feb 17, 2020

Uh oh!

hasufell commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cartazio commented Apr 14, 2020

Uh oh!

l29ah commented Aug 31, 2020

Uh oh!

hasufell commented Aug 31, 2020

Uh oh!

l29ah commented Aug 31, 2020

Uh oh!

cartazio commented Aug 31, 2020

Uh oh!

cartazio commented Sep 1, 2020

Uh oh!

Bodigrim commented Nov 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l29ah commented Nov 15, 2020

Uh oh!

cartazio commented Nov 15, 2020

Uh oh!

Bind `fdopendir` and `openat` #110

Bind `fdopendir` and `openat` #110

hasufell left a comment •

edited

Loading

hasufell Jan 5, 2020 •

edited

Loading

hasufell commented Apr 14, 2020 •

edited

Loading

Bodigrim commented Nov 14, 2020 •

edited

Loading

hs-viktor commented Feb 15, 2021 •

edited

Loading