On Sun, Apr 26, 2020 at 03:30:51PM -0600, Theo de Raadt wrote: > Patrick Wildt <patr...@blueri.se> wrote: > > > I don't know userland very well, so I have a question. In the middle of > > 2019 there have been plenty of changes in regards to changing checks of > > syscalls from < 0 to a more strict == -1, like this one in isakmpd: > > > > ---------------------------- > > revision 1.26 > > date: 2019/06/28 13:32:44; author: deraadt; state: Exp; lines: +2 -2; > > commitid: JJ6Ck4WTrgUiEjJp; > > When system calls indicate an error they return -1, not some arbitrary > > value < 0. errno is only updated in this case. Change all (most?) > > callers of syscalls to follow this better, and let's see if this strictness > > helps us in the future. > > ---------------------------- > > I have about 4000 more changes like that, but I'm stuck with trying to > push it further forward. For various reasons, some of which can be > guessed from this thread. > > > getsockopt(), I think, is also a system call. And the manpage indicates > > that a failure is always -1, and not some arbitrary number: > > > > RETURN VALUES > > Upon successful completion, the value 0 is returned; otherwise the > > value -1 is returned and the global variable errno is set to indicate > > the > > error. > > > > What is the difference between the diff in this mail, and the changes > > done in the middle of last year? > > The difference is this is direct checking of the syscalls. > > Versus checking at a higher layer of abstraction, or conversion of > that result to something else. > > Say you have an interface which returns precisely 0 and -1 for two conditions. > Well then it has a large set of out-of-range values which (a) won't occur > but (b) if they occur, how do you handle them? At which layer? > > The range of numbers returned really express 3 conditions. One which is > impossible, yet if it happens, do you want to convert the impossible to > success, or to failure? > > In the recently supplied diff, a return value of 50 at the system call > layer, is returned into a library returning 0 (success). Furthermore, the > diff itself proposes treating the out-of-range impossible as a success, > and accesses memory which is very probably unintialized. > > > getsockopt() isn't allowed to return > > anything else but 0 and 1, right? Though I guess the current check > > (error != 0) is the one that also catches instances where getsockopt() > > isn't behaving well, even though it shouldn't. But then, with the -1 > > check, wouldn't we be catching more instances of syscalls misbehaving > > if we checked for < -1? > > Correct. I hope you have reached the same indecision point as me. > > I feel uncomfortable changing all checking-points to 3-way decision. > And imagine what a modern compiler would do there... >
The way you put it is obvious how stupid my diff was. I did not understand that it does not call the libc wrapper. Evidently there are still some things going on I do not understand. Sorry for all the inconveniences caused. Best, Martin
