Hi again, While working on a new patch with special cases I realized that file_name_split() can return an empty name too, namely in the case of an existing directory with trailing slashes. This is poorly handled by callers as well. For instance:
$ ln -T existing_file existing_dir/ ln: creating hard link `existing_dir/' => `existing_file': Invalid argument $ ln -sT existing_file existing_dir/ ln: creating symbolic link `existing_dir1/': Invalid argument The attached patch (untested yet) attempts to fix all these cases, by testing for the empty string whenever a call to a name_split() function is made. Unlike the previous one it does not change the interface, but it clarifies the comments in hurd.h. On Sun, Jun 13, 2010 at 10:04 PM, Roland McGrath <[email protected]> wrote: > Right. That does make sense for the RPCs themselves. OTOH for the libc > calls we do want the other errors instead of EINVAL (which IMHO here is > nearly as bizarre to a user as EGRATUITOUS would be), and adding special > cases just feels wrong. I agree it's not ideal, but I'm not sure we really have a choice since the sensible errors to return depend on what is being attempted. Also it turns out it's not so special after all, since everyone needs it ;-) Maybe adding an argument to the __*_name_split() functions would be somewhat cleaner? We would need yet another version to maintain the external interfaces, though. Or maybe just change them and #define the two-arguments form, if this is possible? > [we're free to introduce new errors for non-specified situations] > > On Linux, the errors are: > > mkdir, mknod, link - EEXIST > unlink - EISDIR I beleive on Hurd we would want EPERM since it's what POSIX specifies and what the translators do for non-root directories. > rmdir - EBUSY > > [we should diagnose before RPC] > Those error codes make sense except possibly for the EBUSY case. Among the specified error codes, EBUSY seems to be be the one for which the described circumstances have the closest resemblance to our particular problem, so I used it anyway. I'll test the patch tomorrow and post the results, -- Jérémie Koenig <[email protected]> http://jk.fr.eu.org/
From cc3b65cdb6fd96af6fb591f05f8cba206123d79c Mon Sep 17 00:00:00 2001 From: Jeremie Koenig <[email protected]> Date: Sun, 13 Jun 2010 17:20:48 +0200 Subject: [PATCH] Hurd: Fix split_directory_name(), handle an empty NAME in callers --- ChangeLog | 18 ++++++++++++++++++ hurd/hurd.h | 8 ++++++-- hurd/hurdlookup.c | 2 +- sysdeps/mach/hurd/bind.c | 6 +++++- sysdeps/mach/hurd/link.c | 6 +++++- sysdeps/mach/hurd/linkat.c | 6 +++++- sysdeps/mach/hurd/mkdir.c | 7 ++++++- sysdeps/mach/hurd/mkdirat.c | 7 ++++++- sysdeps/mach/hurd/rename.c | 6 +++++- sysdeps/mach/hurd/rmdir.c | 6 +++++- sysdeps/mach/hurd/symlink.c | 5 ++++- sysdeps/mach/hurd/symlinkat.c | 5 ++++- sysdeps/mach/hurd/unlink.c | 6 +++++- sysdeps/mach/hurd/unlinkat.c | 6 +++++- sysdeps/mach/hurd/xmknodat.c | 5 ++++- 15 files changed, 84 insertions(+), 15 deletions(-) diff --git a/ChangeLog b/ChangeLog index d5f9d30..d222420 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,10 +1,28 @@ +2010-06-13 Jeremie Koenig <[email protected]> + + * hurd/hurdlookup.c: Fix __hurd_split_directory_name() for "/". + * hurd/hurd.h: Document the fact that file_name_split() and + directory_name_split() can return an empty NAME. + * sysdeps/mach/hurd/link.c: Handle an empty NAME from file_name_split() + * sysdeps/mach/hurd/linkat.c: Likewise. + * sysdeps/mach/hurd/symlink.c: Likewise. + * sysdeps/mach/hurd/symlinkat.c: Likewise. + * sysdeps/mach/hurd/xmknodat.c: Likewise. + * sysdeps/mach/hurd/bind.c: Likewise + * sysdeps/mach/hurd/mkdir.c: Handle it from directory_name_split(). + * sysdeps/mach/hurd/mkdirat.c: Likewise. + * sysdeps/mach/hurd/rename.c: Likewise. + * sysdeps/mach/hurd/rmdir.c: Likewise. + * sysdeps/mach/hurd/unlink.c: Likewise. + * sysdeps/mach/hurd/unlinkat.c: Likewise. + 2010-06-02 Emilio Pozuelo Monfort <[email protected]> * hurd/lookup-at.c (__file_name_lookup_at): Accept AT_SYMLINK_FOLLOW in AT_FLAGS. Fail with EINVAL if both AT_SYMLINK_FOLLOW and AT_SYMLINK_NOFOLLOW are present in AT_FLAGS. * hurd/hurd/fd.h (__file_name_lookup_at): Update comment. * sysdeps/mach/hurd/linkat.c (linkat): Pass O_NOLINK in FLAGS. 2010-05-28 Luis Machado <[email protected]> diff --git a/hurd/hurd.h b/hurd/hurd.h index 642ea43..a4270ec 100644 --- a/hurd/hurd.h +++ b/hurd/hurd.h @@ -176,27 +176,31 @@ extern int _hurd_change_directory_port_from_fd (struct hurd_port *portcell, /* Get and set the effective UID set. */ extern int geteuids (int __n, uid_t *__uidset); extern int seteuids (int __n, const uid_t *__uidset); /* Split FILE into a directory and a name within the directory. The directory lookup uses the current root and working directory. If successful, stores in *NAME a pointer into FILE where the name within directory begins and returns a port to the directory; - otherwise sets `errno' and returns MACH_PORT_NULL. */ + otherwise sets `errno' and returns MACH_PORT_NULL. Note that + FILE ending with a slash is not an error, but *NAME will point to + an empty string in this case and passing it to the RPC calls + which manipulate links will fail with EINVAL. */ extern file_t __file_name_split (const char *file, char **name); extern file_t file_name_split (const char *file, char **name); /* Split DIRECTORY into a parent directory and a name within the directory. - This is the same as file_name_split, but ignores trailing slashes. */ + This is the same as file_name_split, but ignores trailing slashes. + Note that if PATH is "/", an empty string can still be stored in *NAME. */ extern file_t __directory_name_split (const char *file, char **name); extern file_t directory_name_split (const char *file, char **name); /* Open a port to FILE with the given FLAGS and MODE (see <fcntl.h>). The file lookup uses the current root and working directory. Returns a port to the file if successful; otherwise sets `errno' and returns MACH_PORT_NULL. */ extern file_t __file_name_lookup (const char *file, int flags, mode_t mode); diff --git a/hurd/hurdlookup.c b/hurd/hurdlookup.c index 8270132..dc40e9b 100644 --- a/hurd/hurdlookup.c +++ b/hurd/hurdlookup.c @@ -168,21 +168,21 @@ __hurd_directory_name_split (error_t (*use_init_port) { error_t addref (file_t crdir) { *dir = crdir; return __mach_port_mod_refs (__mach_task_self (), crdir, MACH_PORT_RIGHT_SEND, +1); } const char *lastslash = strrchr (file_name, '/'); - if (lastslash != NULL && lastslash[1] == '\0') + if (lastslash != NULL && lastslash[1] == '\0' && lastslash > file_name) { /* Trailing slash doesn't count. Look back further. */ /* Back up over all trailing slashes. */ while (lastslash > file_name && *lastslash == '/') --lastslash; /* Find the last one earlier in the string, before the trailing ones. */ lastslash = __memrchr (file_name, '/', lastslash - file_name); } diff --git a/sysdeps/mach/hurd/bind.c b/sysdeps/mach/hurd/bind.c index a42b78a..2f752ac 100644 --- a/sysdeps/mach/hurd/bind.c +++ b/sysdeps/mach/hurd/bind.c @@ -58,21 +58,25 @@ __bind (int fd, __CONST_SOCKADDR_ARG addrarg, socklen_t len) /* Set the node's translator to make it a local-domain socket. */ err = __file_set_translator (node, FS_TRANS_EXCL | FS_TRANS_SET, FS_TRANS_EXCL | FS_TRANS_SET, 0, _HURD_IFSOCK, sizeof _HURD_IFSOCK, MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); if (! err) { /* Link the node, now a socket, into the target directory. */ - err = __dir_link (dir, node, n, 1); + if (n[0] == '\0') + err = EADDRINUSE; + else + err = __dir_link (dir, node, n, 1); + if (err == EEXIST) err = EADDRINUSE; } __mach_port_deallocate (__mach_task_self (), node); if (! err) { /* Get a port to the ifsock translator. */ file_t ifsock = __file_name_lookup_under (dir, n, 0, 0); if (ifsock == MACH_PORT_NULL) { diff --git a/sysdeps/mach/hurd/link.c b/sysdeps/mach/hurd/link.c index f97d7bd..fc05be2 100644 --- a/sysdeps/mach/hurd/link.c +++ b/sysdeps/mach/hurd/link.c @@ -39,21 +39,25 @@ __link (from, to) the receiving filesystem (the one containing TODIR) in dir_link. */ err = __file_getlinknode (oldfile, &linknode); __mach_port_deallocate (__mach_task_self (), oldfile); if (err) return __hurd_fail (err); todir = __file_name_split (to, &toname); if (todir != MACH_PORT_NULL) { - err = __dir_link (todir, linknode, toname, 1); + if (toname[0] == '\0') + err = EEXIST; + else + err = __dir_link (todir, linknode, toname, 1); + __mach_port_deallocate (__mach_task_self (), todir); } __mach_port_deallocate (__mach_task_self (), linknode); if (todir == MACH_PORT_NULL) return -1; if (err) return __hurd_fail (err); return 0; } diff --git a/sysdeps/mach/hurd/linkat.c b/sysdeps/mach/hurd/linkat.c index 062d913..af49fba 100644 --- a/sysdeps/mach/hurd/linkat.c +++ b/sysdeps/mach/hurd/linkat.c @@ -48,21 +48,25 @@ linkat (fromfd, from, tofd, to, flags) the receiving filesystem (the one containing TODIR) in dir_link. */ err = __file_getlinknode (oldfile, &linknode); __mach_port_deallocate (__mach_task_self (), oldfile); if (err) return __hurd_fail (err); todir = __file_name_split_at (tofd, to, &toname); if (todir != MACH_PORT_NULL) { - err = __dir_link (todir, linknode, toname, 1); + if (toname[0] == '\0') + err = EEXIST; + else + err = __dir_link (todir, linknode, toname, 1); + __mach_port_deallocate (__mach_task_self (), todir); } __mach_port_deallocate (__mach_task_self (), linknode); if (todir == MACH_PORT_NULL) return -1; if (err) return __hurd_fail (err); return 0; } diff --git a/sysdeps/mach/hurd/mkdir.c b/sysdeps/mach/hurd/mkdir.c index b7e8074..b669ebe 100644 --- a/sysdeps/mach/hurd/mkdir.c +++ b/sysdeps/mach/hurd/mkdir.c @@ -25,18 +25,23 @@ int __mkdir (file_name, mode) const char *file_name; mode_t mode; { error_t err; const char *name; file_t parent = __directory_name_split (file_name, (char **) &name); if (parent == MACH_PORT_NULL) return -1; - err = __dir_mkdir (parent, name, mode & ~_hurd_umask); + + if (name[0] == '\0') + err = EEXIST; + else + err = __dir_mkdir (parent, name, mode & ~_hurd_umask); + __mach_port_deallocate (__mach_task_self (), parent); if (err) return __hurd_fail (err); return 0; } weak_alias (__mkdir, mkdir) diff --git a/sysdeps/mach/hurd/mkdirat.c b/sysdeps/mach/hurd/mkdirat.c index a300745..d15b300 100644 --- a/sysdeps/mach/hurd/mkdirat.c +++ b/sysdeps/mach/hurd/mkdirat.c @@ -28,16 +28,21 @@ int mkdirat (fd, path, mode) int fd; const char *path; mode_t mode; { error_t err; const char *name; file_t parent = __directory_name_split_at (fd, path, (char **) &name); if (parent == MACH_PORT_NULL) return -1; - err = __dir_mkdir (parent, name, mode & ~_hurd_umask); + + if (name[0] == '\0') + err = EEXIST; + else + err = __dir_mkdir (parent, name, mode & ~_hurd_umask); + __mach_port_deallocate (__mach_task_self (), parent); if (err) return __hurd_fail (err); return 0; } diff --git a/sysdeps/mach/hurd/rename.c b/sysdeps/mach/hurd/rename.c index 181c588..1d8685d 100644 --- a/sysdeps/mach/hurd/rename.c +++ b/sysdeps/mach/hurd/rename.c @@ -32,17 +32,21 @@ rename (old, new) olddir = __directory_name_split (old, (char **) &oldname); if (olddir == MACH_PORT_NULL) return -1; newdir = __directory_name_split (new, (char **) &newname); if (newdir == MACH_PORT_NULL) { __mach_port_deallocate (__mach_task_self (), olddir); return -1; } - err = __dir_rename (olddir, oldname, newdir, newname, 0); + if (oldname[0] == '\0' || newname[0] == '\0') + err = EBUSY; /* the root directory is involved */ + else + err = __dir_rename (olddir, oldname, newdir, newname, 0); + __mach_port_deallocate (__mach_task_self (), olddir); __mach_port_deallocate (__mach_task_self (), newdir); if (err) return __hurd_fail (err); return 0; } diff --git a/sysdeps/mach/hurd/rmdir.c b/sysdeps/mach/hurd/rmdir.c index 46bdf46..2873782 100644 --- a/sysdeps/mach/hurd/rmdir.c +++ b/sysdeps/mach/hurd/rmdir.c @@ -24,18 +24,22 @@ /* Remove the directory FILE_NAME. */ int __rmdir (file_name) const char *file_name; { error_t err; const char *name; file_t parent = __directory_name_split (file_name, (char **) &name); if (parent == MACH_PORT_NULL) return -1; - err = __dir_rmdir (parent, name); + if (name[0] == '\0') + err = EBUSY; + else + err = __dir_rmdir (parent, name); + __mach_port_deallocate (__mach_task_self (), parent); if (err) return __hurd_fail (err); return 0; } weak_alias (__rmdir, rmdir) diff --git a/sysdeps/mach/hurd/symlink.c b/sysdeps/mach/hurd/symlink.c index 857e236..d5ac582 100644 --- a/sysdeps/mach/hurd/symlink.c +++ b/sysdeps/mach/hurd/symlink.c @@ -37,21 +37,24 @@ __symlink (from, to) /* A symlink is a file whose translator is "/hurd/symlink\0target\0". */ memcpy (buf, _HURD_SYMLINK, sizeof (_HURD_SYMLINK)); memcpy (&buf[sizeof (_HURD_SYMLINK)], from, len); dir = __file_name_split (to, &name); if (dir == MACH_PORT_NULL) return -1; /* Create a new, unlinked node in the target directory. */ - err = __dir_mkfile (dir, O_WRITE, 0777 & ~_hurd_umask, &node); + if (name[0] == '\0') + err = EEXIST; + else + err = __dir_mkfile (dir, O_WRITE, 0777 & ~_hurd_umask, &node); if (! err) /* Set the node's translator to make it a symlink. */ err = __file_set_translator (node, FS_TRANS_EXCL|FS_TRANS_SET, FS_TRANS_EXCL|FS_TRANS_SET, 0, buf, sizeof (_HURD_SYMLINK) + len, MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); if (! err) diff --git a/sysdeps/mach/hurd/symlinkat.c b/sysdeps/mach/hurd/symlinkat.c index 9a51c66..bdb69b7 100644 --- a/sysdeps/mach/hurd/symlinkat.c +++ b/sysdeps/mach/hurd/symlinkat.c @@ -44,21 +44,24 @@ symlinkat (from, fd, to) /* A symlink is a file whose translator is "/hurd/symlink\0target\0". */ memcpy (buf, _HURD_SYMLINK, sizeof (_HURD_SYMLINK)); memcpy (&buf[sizeof (_HURD_SYMLINK)], from, len); dir = __file_name_split_at (fd, to, &name); if (dir == MACH_PORT_NULL) return -1; /* Create a new, unlinked node in the target directory. */ - err = __dir_mkfile (dir, O_WRITE, 0777 & ~_hurd_umask, &node); + if (name[0] == '\0') + err = EEXIST; + else + err = __dir_mkfile (dir, O_WRITE, 0777 & ~_hurd_umask, &node); if (! err) /* Set the node's translator to make it a symlink. */ err = __file_set_translator (node, FS_TRANS_EXCL|FS_TRANS_SET, FS_TRANS_EXCL|FS_TRANS_SET, 0, buf, sizeof (_HURD_SYMLINK) + len, MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); if (! err) diff --git a/sysdeps/mach/hurd/unlink.c b/sysdeps/mach/hurd/unlink.c index 959c2f8..010c566 100644 --- a/sysdeps/mach/hurd/unlink.c +++ b/sysdeps/mach/hurd/unlink.c @@ -28,19 +28,23 @@ __unlink (name) const char *name; { error_t err; file_t dir; const char *file; dir = __directory_name_split (name, (char **) &file); if (dir == MACH_PORT_NULL) return -1; - err = __dir_unlink (dir, file); + if (file[0] == '\0') + err = EPERM; + else + err = __dir_unlink (dir, file); + __mach_port_deallocate (__mach_task_self (), dir); if (err) return __hurd_fail (err); return 0; } weak_alias (__unlink, unlink) diff --git a/sysdeps/mach/hurd/unlinkat.c b/sysdeps/mach/hurd/unlinkat.c index 7740c5a..0b03899 100644 --- a/sysdeps/mach/hurd/unlinkat.c +++ b/sysdeps/mach/hurd/unlinkat.c @@ -39,17 +39,21 @@ unlinkat (fd, name, flag) if ((flag &~ AT_REMOVEDIR) != 0) { __set_errno (EINVAL); return -1; } dir = __directory_name_split_at (fd, name, (char **) &file); if (dir == MACH_PORT_NULL) return -1; - err = ((flag & AT_REMOVEDIR) ? __dir_rmdir : __dir_unlink) (dir, file); + if (file[0] == '\0') + err = EPERM; /* root directory */ + else + err = ((flag & AT_REMOVEDIR) ? __dir_rmdir : __dir_unlink) (dir, file); + __mach_port_deallocate (__mach_task_self (), dir); if (err) return __hurd_fail (err); return 0; } diff --git a/sysdeps/mach/hurd/xmknodat.c b/sysdeps/mach/hurd/xmknodat.c index b222759..978b439 100644 --- a/sysdeps/mach/hurd/xmknodat.c +++ b/sysdeps/mach/hurd/xmknodat.c @@ -88,21 +88,24 @@ __xmknodat (int vers, int fd, const char *path, mode_t mode, dev_t *dev) memcpy (bp - len, translator, len); translator = bp - len; len = buf + sizeof (buf) - translator; } dir = __file_name_split_at (fd, path, &name); if (dir == MACH_PORT_NULL) return -1; /* Create a new, unlinked node in the target directory. */ - err = __dir_mkfile (dir, O_WRITE, (mode & ~S_IFMT) & ~_hurd_umask, &node); + if (name[0] == '\0') + err = EEXIST; + else + err = __dir_mkfile (dir, O_WRITE, (mode & ~S_IFMT) & ~_hurd_umask, &node); if (! err && translator != NULL) /* Set the node's translator to make it a device. */ err = __file_set_translator (node, FS_TRANS_EXCL | FS_TRANS_SET, FS_TRANS_EXCL | FS_TRANS_SET, 0, translator, len, MACH_PORT_NULL, MACH_MSG_TYPE_COPY_SEND); if (! err) -- 1.7.1
