FileUnopen: always return a valid file descriptor

We have seen conserver crash due to a buffer overflow which was tracked down to the following code in Spawn():

    if (pCLmall->fd != (CONSFILE *)0) {
        int fd;
        fd = FileUnopen(pCLmall->fd);
        pCLmall->fd = (CONSFILE *)0;
        CONDDEBUG((1, "Spawn(): closing Master() client fd %d", fd));
        close(fd);

FD_CLR(fd, &rinit); FD_CLR(fd, &winit); }

FileUnopen had returned -1 (which can happen for CONSFILEs of type SSLSocket), and that was passed to FD_CLR, which essentially uses it as an array index.

The signature of the crash is as follows:

buffer overflow detected : /usr/sbin/conserver terminated ======= Backtrace: ========= /lib64/libc.so.6(__fortify_fail+0x37)[0x7facde1987a7] /lib64/libc.so.6(+0x116922)[0x7facde196922] /lib64/libc.so.6(+0x118707)[0x7facde198707] /usr/sbin/conserver(+0x158d2)[0x558ddb5468d2] /usr/sbin/conserver(+0x2581a)[0x558ddb55681a] /usr/sbin/conserver(+0x1944f)[0x558ddb54a44f] /usr/sbin/conserver(+0x78f8)[0x558ddb5388f8] /lib64/libc.so.6(__libc_start_main+0xf5)[0x7facde0a2555] /usr/sbin/conserver(+0x7c79)[0x558ddb538c79]

This happens after the server receives a HUP signal.

There are only two callers of FileUnopen, and the above call site is the only one which uses the return value. For that reason, I decided to always return a valid file descriptor instead of changing the caller to check for -1. Note that FileUnopen() could still return -1 in theory:

switch (cfp->ftype) {

... default: retval = -1; break; }

However, after auditing the code, I don't see how we would have a CONSFILE that is not properly initialized with a type. If I missed such a case, then we would also need to modify the caller to check for -1.

bstansell / conserver

FileUnopen: always return a valid file descriptor #95