tish@adiron.UUCP (Tish Todd) (10/15/86)
Index: .../ingres/source/iutil/markopen.c
Description:
A bugs exists in .../ingres/source/iutil/markopen.c
which can cause serious database corruption.
This routine is used in building vaxingres,
which is the main database manipulation routine. The bug exhibits itself
under Ultrix, but not under the 4.2 BSD. The reason for this will
become apparent further down in this explanation.
When vaxingres starts up, it does the following (among other things):
1. Attempts to open a socket and connect to the lock_driver.
If the lock_driver is not active, there is no further problem.
(Note that the 'lock_driver' is the ingres concurrency daemon.)
2. Uses fstat to determine what file descriptors are currently
in use. It presumes that at this point, only essential things,
such as pipes and such, are open. It uses the information from
fstat to build a mask of which descriptors should always remain
open. Everything else is considered throwaway at certain
times. See next step.
The offending code (at about line 36 in markopen.c):
for (i = 0; i < NOFILE; i++)
{
if (fstat(i, &sbuf) >= 0)
*ovect |= 1 << i;
}
3. Using the mask built in step 2, it calls closeall(), which is
a dumb, uncaring brute which merely closes *all* files not
protected by the mask built in step 2. closeall is executed at
various points during vaxingres' lifetime, presumably after any
database operation has been completed (my guess).
The fatal bug appears in step 2, although the symptom shows up later.
fstat does not recognize file descriptors which are open for sockets.
The operation is not supported by Ultrix. fstat returns with negative
number and errno set to EOPNOTSUPP. The code which builds the mask
did not check errno - it only checked the return from fstat.
As a result, the file mask built in markopen() indicated
that the file descriptor for the lock_driver is not open.
Thus, when closeall is invoked, it closes the socket connection to the
lock_driver. However, because closeall - and by extension, vaxingres -
has no smarts, vaxingres still assumes that the socket is alive and
well.
Now, what happens is that vaxingres is told to open a database file.
The open call just happens to use the descriptor previously used for the
lock_driver socket. vaxingres does its thing with the database file ok,
but when it writes to the lock_driver, whammo, the information goes
into the data file instead. Corruption city.
Note that the reason that vaxingres does not exhibit the problem
under the 4.2 BSD is because (at least in our BSD source code) fstat
returns a 0 for this situation. Thus the above check is satisfied
and the descriptor is assumed to be in use.
Fix:
The following change to markopen.c cures the problem (note that
one could also check for errno != EOPNOTSUPP):
for (i = 0; i < NOFILE; i++)
{
if (fstat(i, &sbuf) >= 0)
*ovect |= 1 << i;
/* else if not EBADF - descriptor *might* be in use */
else if (errno != EBADF)
*ovect |= 1 << i;
}stevesu@copper.UUCP (10/24/86)
In article <356@adiron.UUCP>, tish@adiron.UUCP (Tish Todd) quotes code which does stuff like: for (i = 0; i < NOFILE; i++) { do stuff to file descriptor i} and mentions that the code had problems when ported to Ultrix. Unless that NOFILE is a real variable, and not the old #define NOFILE 20 from <sys/param.h>, code like this has more problems than the one Tish mentioned. In 4.3bsd, and in Ultrix, and probably in any number of other recent Unices, the old static limit of 20 file descriptors is no more. In fact, I believe that the maximum number of open file descriptors per process is now a tuneable parameter, so a program can't know it at run time. Any program that does fancy file descriptor manipulation must call getdtablesize(), and be prepared for the return value being arbitrarily large. (I believe that it will never be bigger than 64 for 4.3bsd, but it's clearly stupid for a program to assume that; if you're going to handle arbitrary numbers of file descriptors, why not make your code truly general and maximize portability possibilities in the future?) This change particularly affects callers of select(). The mask arguments are now pointers to arrays of integers, not pointers to single 32-bit integers. (Berkeley really lucked out on this one; they were able to maintain backwards-compatibility with 4.1bsd, thanks in part to the close relationship between pointers and arrays in C which has been so exhaustively discussed in net.lang.c.) Anyway, programs must in general dynamically allocate the arrays they'll hand to select, using getdtablesize() to determine how big the array needs to be. I have used code like (abbreviated for posting): #include "select.h" int *mask = (int *)malloc(NFDwords(getdtablesize()) * sizeof(int)); bzero((int *)mask, NFDwords(getdtablesize()) * sizeof(int)); Setbit(0, mask); select(getdtablesize(), mask, (int *)NULL, (int *)NULL, (struct timeval *)NULL); if(Getbit(0, mask)) ... I use a header file, select.h, which contains some #definitions to make this sort of thing easier. It contains: #define BITSPERINT (sizeof(int) * 8) #define NFDwords(nfds) (((nfds) + BITSPERINT - 1) / BITSPERINT) #define Wordpos(bit) ((bit) / BITSPERINT) #define Bitpos(bit) ((bit) % BITSPERINT) #define Bitmask(bit) (1 << Bitpos(bit)) #define Getbit(bit, mask) (mask[Wordpos(bit)] & Bitmask(bit)) #define Setbit(bit, mask) (mask[Wordpos(bit)] |= Bitmask(bit)) #define Clearbit(bit, mask) (mask[Wordpos(bit)] &= ~Bitmask(bit)) NFDwords(nfds) tells you how many ints it takes to build a bit mask for nfds file descriptors. The rest should be self- explanatory. I'd like to see a header file like this become standard. The names of the macros could certainly be changed; I'm not particularly fond of the ones above. I'm also worried about assuming that there are 8 bits in the "bytes" that sizeof deals with, but any discrepancies for unusual architectures could certainly be encapsulated in select.h, so that #including code wouldn't have to worry about them. I believe that all programs should be written in terms of getdtablesize() (unless P1003 has a better name for it), even if they are being a written for a system for which the static NOFILE would "work." For such systems, just write the following six-liner, and call it getdtab.c or something: #include <sys/param.h> getdtablesize() { return(NOFILE); } Steve Summit tektronix!copper!stevesu