bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: fcntl module (was: O_CLOEXEC support)


From: Bruno Haible
Subject: Re: fcntl module (was: O_CLOEXEC support)
Date: Sat, 22 Aug 2009 13:59:09 +0200
User-agent: KMail/1.9.9

Hi Eric,

> First order of business - we need two modules based on the fcntl name, one
> for the header ... and one for the function.

Yes, absolutely.

Your 3 patches look all fine, except for the mingw replacement of fcntl().

+static int
+dupfd (int fd, int desired_fd)
+{
+  /* Although this looks like unbounded recursion, mingw only supports
+     a maximum of 2047, so stack overflow is unlikely.  */
+  int duplicated_fd = dup (fd);
+  if (duplicated_fd < 0 || desired_fd <= duplicated_fd)
+    return duplicated_fd;
+  else
+    {
+      int r = dupfd (fd, desired_fd);
+      int e = errno;
+      close (duplicated_fd);
+      errno = e;
+      return r;
+    }
+}

I would not risk a recursion depth of 2047. It's time to make this function
non-recursive. How about this?

static int
dupfd (int fd, int desired_fd)
{
  unsigned char fds_to_close[4096 / 8];
  unsigned int fds_to_close_bound = 0;
  int result;

  /* Call dup(), keeping the returned fds open, until a result is
     >= desired_fd.  */
  for (;;)
    {
      int duplicated_fd = dup (fd);
      unsigned int index;

      if (duplicated_fd < 0 || desired_fd <= duplicated_fd)
        {
          result = duplicated_fd;
          break;
        }

      index = (unsigned int) duplicated_fd / 8;
      if (index >= fds_to_close_bound)
        {
          if (index >= fds_to_close)
            /* Need to increase the size of fds_to_close.  */
            abort ();
          memset (fds_to_close + fds_to_close_bound, 0, index + 1 - 
fds_to_close_bound);
          fds_to_close_bound = index + 1;
        }
      fds_to_close[index] |= 1 << ((unsigned int) duplicated_fd % 8);
    }

  /* Close the previous fds that turned out to be too small.  */
  {
    int saved_errno = errno;
    unsigned int duplicated_fd;

    for (duplicated_fd = 0; duplicated_fd < fds_to_close_bound * 8; 
duplicated_fd++)
      if ((fds_to_close[duplicated_fd / 8] >> (duplicated_fd % 8)) & 1)
        close (duplicated_fd);
    errno = saved_errno;
  }

  return result;
}

Then about F_SETFD:

+    case F_SETFD:
+      {
+       int value = va_arg (arg, int);
+       if (value != 0 && value != FD_CLOEXEC)
+         errno = EINVAL;
+       else
+         {
+           HANDLE handle = (HANDLE) _get_osfhandle (fd);
+           DWORD flags = value ? 0 : HANDLE_FLAG_INHERIT;
+           if (handle == INVALID_HANDLE_VALUE
+               || SetHandleInformation (handle, HANDLE_FLAG_INHERIT,
+                                        flags) == 0)
+             errno = EBADF;
+           else
+             result = 0;
+         }

This code does not preserve the invariant that for any open fd, the flags
stored inside MSVCRT for fd contain the bit FNOINHERIT if and only if
the handle _get_osfhandle (fd) has the InheritHandle flag set to FALSE.

Recall that for an fd to be passed to a child process, two things must
occur:
  1) _dospawn must pass the handle as part of a certain array to CreateProcess;
     this is done only if the (hidden) flags contain FNOINHERIT, and
  2) CreateProcess must transform the handle valid for the parent to a handle
     valid for the child. This is done if the InheritHandle flag is set.
     If it is not set, the child process gets an INVALID_HANDLE_VALUE.

But the flags stored inside MSVCRT for fd cannot be directly accessed.
The only way they can be influenced is through the _open_osfhandle
function. So, the only viable implementation of F_SETFD would be to
  1. Determine the flags for _open_osfhandle.
     GetHandleInformation, then translate
       !HANDLE_FLAG_INHERIT -> O_NOINHERIT,
     setmode(O_BINARY) == O_TEXT -> O_TEXT and redo setmode(O_TEXT),
     O_APPEND unknown!
  2. Use dup and close to move the fd to a different value,
  3. Use repeated equivalents of w32spawn.h:dup_noinherit to copy the
     moved fd to the right place.
  4. Close fds that were returned by dup_noinherit but were the wrong
     value.

But this cannot work with O_APPEND and is quite a hack.

I therefore think that instead of offering the POSIXy fcntl call,
gnulib should offer functions like dup, dup2, dup_safer, fd_safer,
that take a flags argument as additional parameter.
  int dup_ex (int fd, int flags);
  int dup2_ex (int oldfd, int newfd, int flags);
  int dup_safer_ex (int fd, int flags);
  int fd_safer_ex (int fd, int flags);

The possible values in flags include not only O_CLOEXEC alias O_NOINHERIT,
but also O_BINARY / O_TEXT and O_APPEND.

Bruno




reply via email to

[Prev in Thread] Current Thread [Next in Thread]