On Sat, Sep 16, 2023 at 9:47 AM Ingo Schwarze <schwa...@usta.de> wrote:
> Janne Johansson wrote on Sat, Sep 16, 2023 at 11:49:10AM +0200: > > > In case someone wants the change in a diff format, those 5 seconds > > of work are available here: > > http://c66.it.su.se:8080/obsd/unveil.2.diff > > How come jj@ forgot we want diffs inline? <scratching my head> > > > +.It Bq Er ENOTDIR > > +.Fa path > > +points to a file that is not a directory. > > That diff is certainly not OK. > It is misleading because a "path" argument pointing to a file > that is not a directory is actually valid. > > The problem only occurs when you add a trailing slash to the name > of a file that is not a directory, like this: > > $ touch tmp.txt > $ ls -l tmp.txt > -rw-r--r-- 1 schwarze schwarze 0 Sep 16 18:00 tmp.txt > $ ls -l tmp.txt/ > ls: tmp.txt/: Not a directory > $ ls -l tmp.txt/foobar > ls: tmp.txt/foobar: Not a directory > > Looking at sys_unveil in /sys/kern/vfs_syscalls.c, i suspect that > errno actually comes from a lower layer, likely namei(9), rather than > from unveil(2) itself. > Yes, from namei(9)/vfs_lookup(9) and the VOP_LOOKUP() calls they make. Basically all the syscalls that take a path should document the standard set of errnos that paths can prompt: ENOTDIR, ENAMETOOLONG, ENOENT, EACCES, ELOOP, EIO, (and EFAULT I guess). Those in the *at() family should document EBADF, and the fd-related causes of ENOTDIR and EACCES. And then there are additional errors that those calls which may create or modify the target path probably need to document: EISDIR and EROFS, maybe ENXIO or EPERM... > In library manual pages, we occasionally have sentences like this: > > execv() may fail and set errno for any of the errors specified > for the library function execve(2). > > The fgetln() function may also fail and set errno for any of the > errors specified for the routines fflush(3), malloc(3), read(2), > stat(2), or realloc(3). > > But it doesn't look like we do that in system call manuals, and > besides, namei(9) appears to lack the RETURN VALUES section. > Hmm, would it be useful to fill in namei(9)'s RETURN VALUES section with the template of the "so you took a path argument..." errnos that syscall manpages can start from? > So, what is our policy with syscall ERRORS sections? > For standardized syscalls, POSIX probably needs to be taken > into account in addition to our implementation. > But for our very own syscalls? Hm... > > access(2), acct(2), chdir(2), chflags(2), chmod(2), chown(2), chroot(2), > execve(2), getfh(2), ktrace(2), link(2), mkdir(2), mkfifo(2), mknod(2), > mount(2), open(2), pathconf(2), quotactl(2), readlink(2), rename(2), > revoke(2), rmdir(2), stat(2), statfs(2), swapctl(2), symlink(2), > truncate(2), unlink(2), utimes(2) all list ENOTDIR, and i don't see > any right now taking a path argument that don't. > > Do we want the following diff? > Yes, I think we want at least that. ok guenther@ > I did not sort the existing return values in alphabetical order, > > there were two out of order in there, but that is a separate > > commit I guess. > > NetBSD has a policy of sorting ERRORS entries alphabetically, we don't. > > Yours, > Ingo > > > Index: unveil.2 > =================================================================== > RCS file: /cvs/src/lib/libc/sys/unveil.2,v > retrieving revision 1.22 > diff -u -r1.22 unveil.2 > --- unveil.2 6 Sep 2021 08:03:08 -0000 1.22 > +++ unveil.2 16 Sep 2023 16:38:24 -0000 > @@ -158,6 +158,10 @@ > A directory in > .Fa path > did not exist. > +.It Bq Er ENOTDIR > +A component of the > +.Fa path > +prefix is not a directory. > .It Bq Er EINVAL > An invalid value of > .Fa permissions > >
