[Gpg4win-devel] kdpipeio close buglet

[Marcus Brinkmann]

Hi,

I am currently looking through w32-qt-io.c to make sure it gets the
same bugfixes (if necessary) that we applied to the w32-glib-io.c
backend last week.

The short story is that I found a bug in that kdpipeiodevice does not
DuplicateHandle the _get_osfhandle (fd) at initialization time.
_get_osfhandle does not acquire a reference for the handle for you, so
you call CloseHandle before _close(), which will call CloseHandle
again, and you end up closing the same handle twice with only one
reference.

Here is the long story, with additional notes and some suggestions how
the double close can be fixed.

* MAX_SLAFD 50000

  This seems excessive.  libc handles are guaranteed to be small A
  smaller constant can catch libc vs osfhandle confusion (together
  with the bound checks), and saves a bit of memory.

  Suggestion: Lower to eg 1024.

* DeviceEntry: I can't see how actual_fd can ever be something else
  than the index of the pointer to this entry in the table.  So, this
  member seems redundant.  Suggestion: let's remove it.

* _gpgme_io_close: The loop over all entries in the iodevice_table
  seems unnecessary, as an entry should never be copied without
  creating a new reference.  Should probably be removed or replaced by
  an assert (but the loop for an assert is somewhat costly).

  You call iodev->close() and _close (fd).  In kdpipeiodevice close
  you do a CloseHandle on handle, which is initialized to
  _get_osfhandle (fd).  But _get_osfhandle does not acquire a
  reference for the handle, so the CloseHandle actually releases the
  libc file descriptor's internal reference.

  Three possible solutions: 1) kdpipeiodevice could acquire its own
  reference by calling DuplicateHandle, or it could 2) swallow the FD,
  then it (rather than _gpgme_io_close) should call _close, or it
  could 3) rely on the caller to keep the reference and not close the
  handle.  glib does take approach 2).  It seems to me that approach
  1) fits best in the current code.

* For _gpgme_io_dup, it can be assumed here that FD was previously
  created with _gpgme_io_pipe, so the iodevice should already exist.
  I suggest we replace this by an assert.

  It is correct to share the iodevice for dup'ed handles.  This is not
  strictly following the Unix semantics of dup, so the function name
  is a misnomer.  My bad (see priv-io.h for some details).

  Note that GPGME guarantees that the dup'ed FD is closed before the
  FD from which it was dup'ed.  This is important, because the
  channels are shared, and for glib closing the FD implies destroying
  the channel (due to the swallowing approach taken at initialization
  time).  If you take approach 1), this would not be an issue.

  In any case, the FD dup you create here must be closed in
  _gpgme_io_close.  If you choose to use DuplicateHandle in
  kdpipeiodevice (the first approach above), then it is correct to
  call _close in _gpgme_io_close for all FDs.  In the glib approach
  (swallow FD) we remember the channel fd and skip the _close for that
  FD only.

  (Note to self and Werner: We could _dup the FD before creating the
  channel, and then simply use glib's close_on_unref mode and glib's
  reference counting to keep track of users, thereby taking approach
  1) rather than approach 2).

Thanks,
Marcus