Svante Signell, le Sat 21 Feb 2015 16:09:46 +0100, a écrit :
> Most glib2.0 and dbus tests pass (after bootstrapping).
Most, i.e. not all? Are the failing ones related with SCM_CREDS? Was
the synchronous/asynchronous issue solved?
> + /* FIXME: Currently only ONE port is supported, error out if more */
> + if (ncreds != 0 && ncreds != 1)
> + {
> + /* FIXME: Which error to issue here? */
> + err = EGRATUITOUS;
> + goto out;
> + }
I don't really understand: is it really hard to support more than one
SCM_CREDS? I haven't looked at the details, but it seems to me your
implementation would already be almost working fine for more than one
SCM_CREDS, i.e. it seems to be only missing accessing cports[something]
instead of cports[0], and using an array of rendezvous ports instead of
just one.
> + idx[k] = 'c';
It seems quite cumbersome to me to be going through an idx array:
can't all the loops just work on the single ports array? AIUI it's a
matter of just stuffing the SCM_CREDS part inside the existing loop for
SCM_RIGHTS. That would also save going through the messages several
times. Same for the receiving side.
> +#define UIDSIZE sizeof (uid_t) * CMGROUP_MAX
> +#define GIDSIZE sizeof (gid_t) * CMGROUP_MAX
> + __uid_t euids_buf[UIDSIZE], auids_buf[UIDSIZE];
> + __gid_t egids_buf[GIDSIZE], agids_buf[GIDSIZE];
?? sizes of the array are in number of elements, not in bytes.
> + HURD_CRITICAL_BEGIN;
> + __mutex_lock (&_hurd_id.lock);
Why taking these?
> + /* Check IDs */
> + if (euid != euids_buf[0] || auid != auids_buf[0] ||
> + egid != egids_buf[0] || agid != agids_buf[0])
> + {
> + err = EPERM;
I'm not sure we should return EPERM here: the problem is not that the
receiving side didn't have some permission, but that the sending side
lied. Perhaps EIO would be more appropriate?
> + /* FIXME: is sorted check needed? */
> + if (groups[i] != egids_buf[i])
Mmm. AIUI it should be already in the right order: on the sending side,
__getgroups will return them in the same order as what auth would
provide.
> + /* Check PID */
> + /* XXX: Use proc_getallpids and proc_getprocinfo until
> + proc_user_authenticate proc_server_authenticate is implemented
s/authenticate/identify/
> + err = __USEPORT (PROC, __proc_getallpids (port, &pids, &npids));
...
> + err = __USEPORT (PROC, __proc_getprocinfo (port, pids[i], &flags,
...
> + if (pi->owner == euid)
This is not completely what we want to check, but it's probably fine for
a first implementation: even if we don't check the pid precisely, we
check that the pid belongs to the same user. Please document that in
comments along the TODO.
> + /* FIXME: which return code to use?
> + EACCES = permission denied
> + ESRCH = no such process
> + */
> + err = ESRCH;
Same as above: the client lied.
> nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
> - / sizeof (int);
> + / sizeof (int);
Avoid space-changing hooks.
> - if (cmsg->cmsg_level == SOL_SOCKET
> - && cmsg->cmsg_type == SCM_RIGHTS)
> + if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type ==
> SCM_RIGHTS)
Ditto. And even more so since it makes it longer than 80 characters!
> - nfds = (cmsg->cmsg_len
> - - CMSG_ALIGN (sizeof (struct cmsghdr)))
> - / sizeof (int);
> - for (i = 0; i < nfds && j < nports; i++, j++)
> + nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
> + / sizeof (int);
> + for (i = 0; i < nfds && j < nrights; i++, j++)
Ditto. This even hides some change in the code, this is really not
something to do for proper review.
> + if (err)
> + return __hurd_fail (err);
> + }
> +
> __vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
Take care of proper complete deallocations on error.
Samuel
