Peter Tribble wrote: > On Mon, 2006-04-24 at 20:50, Dave Miner wrote: >>>>> 5. 6322837 >>>>> >>>>> I've added a simple check that the input file specified is indeed a >>>>> regular file. >>>>> >>>>> Note that this actually constitutes a change of behaviour, and will >>>>> cause anything that explicitly uses /dev/stdin (rather than the more >>>>> conventional notation - see point 3 above) to fail. I have identified 2 >>>>> places where this occurs >>>>> >>>>> /sfw/usr/src/pkgdefs/common_files/checkinstall.initd >>>>> /on/usr/src/pkgdefs/common_files/checkinstall.initd >>>>> >>>> Ouch; synchronizing fixing those two would be painful, we might also >>>> need an ARC fasttrack to allow this change. How about adding an S_ISCHR >>>> as well, which would continue to allow /dev/stdin to work? >>> That idea falls over. You actually need S_ISFIFO for stdin, and that >>> doesn't work very well either. My preferred solution (after trying >>> a number of variations that didn't work) is simply to check for the >>> "/dev/stdin" string explicitly. Diff from my original: >>> >> I'm still hoping for something less, um, special. Can you say more >> about the variation you tried that didn't work? I'm wondering if >> !S_ISDIR() (maybe && !S_ISBLK()) is the better choice, since it would >> catch pretty much all the obviously stupid errors. > > As I said, it turns out you need S_ISFIFO for /dev/stdin. Just > allowing S_ISFIFO as well as S_ISCHR failed, because you > then do the ELF check and can't rewind the fifo. >
The rewind could be worked around. But at that point, if it were the ELF check in the way, I'd be more keen to just drop the ELF check instead. There are plenty of other file formats that are equally unusable, and if the rest of the code doesn't deal with that well, then that's a better issue to focus on. > So you could explicitly allow a FIFO as an extra branch to the if > statement [so S_ISFIFO() rather than strcmp(file, "/dev/stdin")], > but then I have to ask what fifo other than stdin would actually > be valid. > I don't know offhand, but that's somewhat the point here - we know of one special case, /dev/stdin, and I'd much rather not be back here having to add more special cases after we've caused an escalation. We want this to be stable for the foreseeable future (modulo other RFE's such as the one you suggested below). > As far as I can see, going down that branch - and doing the > weird ELF check - is only valid for regular files anyway. Why > allow doors, sockets, or event ports? It just seems easier > to me to allow the right things that to try and enumerate > the bad cases. > The problem is we know the obviously bad cases, but we don't necessarily know all the good cases that may have been allowed by the existing code. So I tend to focus on doing what we know is correct, rather than guessing on what might not cause a problem. > I'm quite happy with whatever comes out, though. > > (I just had a horrid thought about hacking it so that if > it was a directory, it opened the directory and generated > a directory listing of files to check - perhaps we had > better make a decision before I start thinking any more.) > Perhaps an interesting feature to add, but that's another RFE we could handle later. Don't stop thinking ;-) Dave
