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 <open...@academicsolutions.ch> 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 -0000 1.1
> > +++ net/getpeereid.c 26 Apr 2020 20:28:50 -0000
> > @@ -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;
> >
>
