Bug#704608: apt: FileFd issues

[Niels Thykier <niels@thykier.net>]

Package: apt
Severity: normal

Reviewing some APT code I noticed a couple of possible issues in
FileFd.

"""
In FileFd::OpenDescriptor(int Fd, unsigned int const,
  APT::Configuration::Compressor const &, bool):

  [...]
  Flags = (AutoClose) ? FileFd::AutoClose : 0;
  if (AutoClose == false && [... compress is one of gzip/bzip2...])
  {
      // Need to duplicate fd here or gzclose for cleanup will close the fd as well
      iFd = dup(Fd);
  }
  else
     iFd = Fd;
  [...]
  if (OpenInternDescriptor(Mode, compressor) == false)
  {
     if (AutoClose)
        close (iFd);
     return _error->Errno("gzdopen",_("Could not open file descriptor %d"), Fd);
  }
  return true;
"""

If OpenInternDescriptor fails, iFd will be closed if AutoClose is true
but it is not set to -1.  Since Flags have been set to
FileFd::AutoClose, the destructor will (via Close()) attempt to close
iFd again.  This is like to fail with EBADF given it was already
closed.  This given the (minor?) side-effect of causing an error being
pushed on to the _error handler (but otherwise should not cause any
issues AFAICT).

The auto-close behaviour of OpenDescriptor(...) is in general
inconsistent in regard to the input "fd".  The method above, it will
close the input file descriptor (due to iFd=Fd).  But the other
OpenDescriptor method will not close that handle on error.  This is
very unhealthy because the caller has no way of knowing what state the
fd will be in on error[2].
  At least OpenMaybeClearSignedFile appears to rely on the fd being
closed on error by OpenDescriptor.

~Niels

[1] Here I could have used a comment saying there is no need to check
the return value of dup because that OpenInternDescriptor will
eventually do it for us.  Anyway.

[2] Admittedly the other errors seem unlikely to occur in any
"pratical" use of OpenDescriptor, but still.