Xref: utzoo comp.bugs.4bsd:1730 comp.lang.c:35793
Path: utzoo!utgpu!news-server.csri.toronto.edu!cs.utexas.edu!uwm.edu!rpi!uupsi!cmcl2!kramden.acf.nyu.edu!brnstnd
From: brnstnd@kramden.acf.nyu.edu (Dan Bernstein)
Newsgroups: comp.bugs.4bsd,comp.lang.c
Subject: Re: Complexity of reallocating storage (was users command crap)
Message-ID: <10210:Feb505:10:2991@kramden.acf.nyu.edu>
Date: 5 Feb 91 05:10:29 GMT
References: <1991Feb2.045119.22199@zoo.toronto.edu> <14994:Feb207:10:4791@kramden.acf.nyu.edu> <27AD7A46.343B@tct.uucp>
Organization: IR
Lines: 72

Summary: Chip complains that a read() from disk into a ``newuid''
integer may fail and hence have pty switch to the wrong uid. But I
explicitly initialized newuid to uid just to handle this possibility.
Can people stop taking my code out of context?

Fgs, right above the section that Geoff quoted is the comment

  /* XXX: We should have some error recovery here! */

I freely admit that the code isn't perfect. But I already have enough
safeguards in place that I'm not worried about any huge disasters, and I
don't need a dozen people telling me that my programming technique has
problems when they haven't even bothered to read my code.

In article <27AD7A46.343B@tct.uucp> chip@tct.uucp (Chip Salzenberg) writes:
> >>    (void) read(fdsess,(char *) &newuid,sizeof(int));
> That particular "fatal" error -- disk failure -- will not kill the
> program.  Instead, it will result in a pty program continuing to run
> using wrong data that could have been detected and ignored.  This is
> "good programming practice?"

So how would you like to handle it, Chip? Die with a fatal error,
possibly killing a truly critical system program running under pty?

You should already have known that the value read in---newuid---is
initialized to uid. Now I know there are some wacko UNIX systems out
there, but I've never seen an I/O error on the first block of a disk
file that resulted in read() returning less than 4 bytes of that block.
Either the entire block gets read in, or none of it does. (Sure, it's
still worth testing for the error---if you can figure out a sane way to
handle it.)

So, Chip, all that will happen upon an I/O error in that section is that
pty will continue with the same uid as it had before. If communication
(or the disk) breaks down entirely then the signaller process will lose
track of the master process, but it is practically impossible that the
section of code you quoted will result in a wrong uid, even in the
already disastrous situation that there are physical disk errors. You
say ``it will result in a pty program continuing to run using wrong
data''; I say that you don't know what you're talking about.

I put that newuid = uid initialization in for a reason, and I am getting
rather sick of people drawing incorrect conclusions about my code
because Geoff took it out of context.

Nobody's programming is perfect; the best I hope I can say is that I've
gotten better over the years. I do think, however, that I have some
degree of common sense about what can fail and what can't, and when I've
already put a comment into my program saying ``We should test for foo''
it gets rather annoying to see a dozen people saying ``Dan, you don't
use good programming technique, because you don't test for foo. Don't
you realize what a programming criminal you are?'' Sheesh. Don't you
read code before you criticize it?

> >>    if (chdir(newsuid) == -1)
> >>      (void) mkdir(newsuid,0700);
> >This cannot fail unless some renegade sysadmin changes the mode of the
> >session directory while pty is running.
> Not to protect against that possibility is a strong temptation, but it
> certainly is not "good programming practice."

Well, golly gee, I suppose every program should encrypt all its internal
data and throw away the key, just in case some renegade sysadmin is
trying to corrupt its operation with ptrace()! Yeah!

Chip, be reasonable. You can't demand of system programs that they check
for external system consistency at every step. Defensive programming is
fine, but screwing around with correct code just because ``a sysadmin
might go out of his way to damage the internal state of the system''
can be pretty stupid.

---Dan