Re: [patch] Check for -1 explicitly in getpeereid.c
On Sun, Apr 26, 2020 at 03:30:51PM -0600, Theo de Raadt wrote: > Patrick Wildt 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
Re: [patch] Check for -1 explicitly in getpeereid.c
Patrick Wildt 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...
Re: [patch] Check for -1 explicitly in getpeereid.c
Hi, 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. 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? 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? Patrick On Sun, Apr 26, 2020 at 02:45:54PM -0600, Theo de Raadt wrote: > If it returns 50 then the creds structure is not valid, and can't be copied > from. It only returns valid creds *IF* success is indicated by 0. But then > you convert 50 to a return value of 0, and hide any indication that things > went weird? > > No way, I'm not buying your argument. I think the code is making the > correct choice now. > > Martin Vahlensieck wrote: > > > Hi there > > > > From the getsockopt(2) manual page says getsockopt(2) returns -1 on > > error and 0 on success. Also getpeereid(3) only lists those 2 values. > > This diff makes the return value check in getpeereid explicit. I guess > > this is how it is done elsewhere in the tree (there is a commit turning > > a bunch of "... < 0" to "== -1" I think this falls under that category). > > > > Best, > > > > Martin > > > > Index: net/getpeereid.c > > === > > RCS file: /cvs/src/lib/libc/net/getpeereid.c,v > > retrieving revision 1.1 > > diff -u -p -r1.1 getpeereid.c > > --- net/getpeereid.c1 Jul 2010 19:15:30 - 1.1 > > +++ net/getpeereid.c26 Apr 2020 20:28:50 - > > @@ -28,7 +28,7 @@ getpeereid(int s, uid_t *euid, gid_t *eg > > > > error = getsockopt(s, SOL_SOCKET, SO_PEERCRED, > > &creds, &credslen); > > - if (error) > > + if (error == -1) > > return (error); > > *euid = creds.uid; > > *egid = creds.gid; > > >
Re: [patch] Check for -1 explicitly in getpeereid.c
If it returns 50 then the creds structure is not valid, and can't be copied from. It only returns valid creds *IF* success is indicated by 0. But then you convert 50 to a return value of 0, and hide any indication that things went weird? No way, I'm not buying your argument. I think the code is making the correct choice now. Martin Vahlensieck wrote: > Hi there > > From the getsockopt(2) manual page says getsockopt(2) returns -1 on > error and 0 on success. Also getpeereid(3) only lists those 2 values. > This diff makes the return value check in getpeereid explicit. I guess > this is how it is done elsewhere in the tree (there is a commit turning > a bunch of "... < 0" to "== -1" I think this falls under that category). > > Best, > > Martin > > Index: net/getpeereid.c > === > RCS file: /cvs/src/lib/libc/net/getpeereid.c,v > retrieving revision 1.1 > diff -u -p -r1.1 getpeereid.c > --- net/getpeereid.c 1 Jul 2010 19:15:30 - 1.1 > +++ net/getpeereid.c 26 Apr 2020 20:28:50 - > @@ -28,7 +28,7 @@ getpeereid(int s, uid_t *euid, gid_t *eg > > error = getsockopt(s, SOL_SOCKET, SO_PEERCRED, > &creds, &credslen); > - if (error) > + if (error == -1) > return (error); > *euid = creds.uid; > *egid = creds.gid; >
[patch] Check for -1 explicitly in getpeereid.c
Hi there >From the getsockopt(2) manual page says getsockopt(2) returns -1 on error and 0 on success. Also getpeereid(3) only lists those 2 values. This diff makes the return value check in getpeereid explicit. I guess this is how it is done elsewhere in the tree (there is a commit turning a bunch of "... < 0" to "== -1" I think this falls under that category). Best, Martin Index: net/getpeereid.c === RCS file: /cvs/src/lib/libc/net/getpeereid.c,v retrieving revision 1.1 diff -u -p -r1.1 getpeereid.c --- net/getpeereid.c1 Jul 2010 19:15:30 - 1.1 +++ net/getpeereid.c26 Apr 2020 20:28:50 - @@ -28,7 +28,7 @@ getpeereid(int s, uid_t *euid, gid_t *eg error = getsockopt(s, SOL_SOCKET, SO_PEERCRED, &creds, &credslen); - if (error) + if (error == -1) return (error); *euid = creds.uid; *egid = creds.gid;
