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.
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.
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.
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.)
> > *** main.c.firstpass Tue Apr 18 19:11:32 2006
> > --- main.c Sun Apr 23 20:26:14 2006
> > ***************
> > *** 530,535 ****
> > --- 530,537 ----
> >
> > if (strcmp(file, "-") == 0) {
> > fplist = stdin;
> > + } else if (strcmp(file, "/dev/stdin") == 0) {
> > + fplist = stdin;
> > } else {
> > if ((fd = open(file, O_RDONLY)) == -1) {
> > progerr(gettext(ERR_IOPEN), file);
> >
> > This captures the exact usage, and doesn't allow other peculiar
> > file to get through the trap.
--
-Peter Tribble
L.I.S., University of Hertfordshire - http://www.herts.ac.uk/
http://www.petertribble.co.uk/ - http://ptribble.blogspot.com/
