On Thu, Aug 29, 2024 at 08:47:27AM +1000, Richard Henderson wrote:
> On 8/28/24 22:48, Daniel P. Berrangé wrote:
> > > dir = opendir("/proc/self/fd");
> >
> > IIUC from previous threads this is valid on Linux and on Solaris.
> >
> > On FreeBSD & macOS, you need /dev/fd though.
>
> Fair, but importantly, it doesn't do anything *incorrect* those systems: it
> merely skips this method with ENOENT.
>
> > > + int open_max = sysconf(_SC_OPEN_MAX), i;
> > > +
> > > + /* Fallback */
> > > + for (i = 0; i < open_max; i++) {
> > > + close(i);
> > > + }
> >
> > I'm told that sysconf(_SC_OPEN_MAX) returns -1 on some versions of
> > macOS. "Luckily" since we assigned to 'int' rather than 'unsigned int'
> > this will result in us not closing any FDs in this fallback path,
> > rather than trying to close several billion FDs (an effective hang).
>
> Ouch.
>
> >
> > If _SC_OPEN_MAX returns -1, we should fallback to the OPEN_MAX
> > constant on macOS (see commit de448e0f26e710e9d2b7fc91393c40ac24b75847
> > which tackled a similar issue wrt getrlimit), and fallback to perhaps
> > a hardcoded 1024 on non-macOS.
>
> I wish the timing on this had been better -- 25 minutes earlier and I would
> have delayed rc4.
>
> Since macOS simply doesn't close fds, I'm of a mind to release 9.1.0 without
> this, and fix it for 9.1.1. Thoughts?
The original net/tap.c code for closing FDs has the same bug, so in that
area we do NOT have a regression.
The original async teardown code would fail to close FDs as it is looking
for close_range or /proc/$PID/fd, and has no sysconf fallback, so again
no regression there.
IOW, I think this is acceptable to fix in 9.1 stable.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
