|

»

freebsd-hackers

rmdir(2) and mkdir(2) both return EISDIR for argument "/"

View: New views

15 Messages — Rating Filter: Alert me

	rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message hi there, i dug up this old pr http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 and was surprised it still remains valid for 9-CURRENT. indeed running the following code: #include <sys/stat.h> #include <errno.h> #include <stdio.h> #include <unistd.h> main() { rmdir("/"); printf("rmdir errno: %d\n", errno); mkdir("/", 511); printf("mkdir errno: %d\n", errno); } returns: rmdir errno: 21 mkdir errno: 21 which is EISDIR. as the pr already said EISDIR isn't mentioned at all in mkdir(2) or rmdir(2). so i guess this should either get fixed or we add a "BUGS" section to both manuals where this problem gets mentioned. alex _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alex Dupre :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Alexander Best ha scritto: > i dug up this old pr http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 I think the EISDIR error is coming from kern/vfs_lookup.c, lookup() function with cn_nameptr = "": /* * Check for degenerate name (e.g. / or "") * which is a way of talking about a directory, * e.g. like "/." or ".". */ if (cnp->cn_nameptr[0] == '\0') { ... if (cnp->cn_nameiop != LOOKUP) { error = EISDIR; goto bad; } ... -- Alex Dupre _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Alex Dupre schrieb am 2009-11-06: > Alexander Best ha scritto: > > i dug up this old pr > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 > I think the EISDIR error is coming from kern/vfs_lookup.c, lookup() > function with cn_nameptr = "": > /* > * Check for degenerate name (e.g. / or "") > * which is a way of talking about a directory, > * e.g. like "/." or ".". > / > if (cnp->cn_nameptr[0] == '\0') { > ... > if (cnp->cn_nameiop != LOOKUP) { > error = EISDIR; > goto bad; > } > ... thanks a lot for finding the problem in the src. what do you think of the patch attached to this message? after applying it the example code i posted in my previous message returns the following output (instead of EISDIR): rmdir errno: 16 (which is EBUSY) mkdir errno: 17 (which is EEXIST) i don't know if these really are the correct return values, but it's what the originator of the PR requested. alex --- vfs_lookup.c 2009-11-06 16:14:41.000000000 +0100 +++ /usr/src/sys/kern/vfs_lookup.c 2009-11-06 16:13:19.000000000 +0100 @@ -35,7 +35,7 @@ / #include <sys/cdefs.h> -__FBSDID("$FreeBSD$"); +__FBSDID("$FreeBSD: head/sys/kern/vfs_lookup.c 195939 2009-07-29 07:44:43Z rwatson $"); #include "opt_kdtrace.h" #include "opt_ktrace.h" @@ -563,8 +563,12 @@ error = ENOTDIR; goto bad; } - if (cnp->cn_nameiop != LOOKUP) { - error = EISDIR; + if (cnp->cn_nameiop != LOOKUP && cnp->cn_nameiop == DELETE) { + error = EBUSY; + goto bad; + } + if (cnp->cn_nameiop != LOOKUP && cnp->cn_nameiop == CREATE) { + error = EEXIST; goto bad; } if (wantparent) { _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alex Dupre :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Alexander Best ha scritto: > thanks a lot for finding the problem in the src. what do you think of the > patch attached to this message? I'm sorry, but I haven't enough knowledge about this subsystem to judge your patch. Someone else should jump in. -- Alex Dupre _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Alex Dupre schrieb am 2009-11-06: > Alexander Best ha scritto: > > thanks a lot for finding the problem in the src. what do you think > > of the > > patch attached to this message? > I'm sorry, but I haven't enough knowledge about this subsystem to > judge > your patch. Someone else should jump in. neither do i. ;) i fear the patch is just a dirty hack and probably breaks tons of conventions. ;) i'll post the patch as followup to the PR and see what happens. thanks again for having a look at the problem and pointing out the responsible code. alex _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Gary Jennejohn-2 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Fri, 06 Nov 2009 16:32:22 +0100 (CET) Alexander Best <alexbestms@...> wrote: > Alex Dupre schrieb am 2009-11-06: > > Alexander Best ha scritto: > > > i dug up this old pr > > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 > > > I think the EISDIR error is coming from kern/vfs_lookup.c, lookup() > > function with cn_nameptr = "": > > > > /* > > * Check for degenerate name (e.g. / or "") > > * which is a way of talking about a directory, > > * e.g. like "/." or ".". > > */ > > if (cnp->cn_nameptr[0] == '\0') { > > ... > > if (cnp->cn_nameiop != LOOKUP) { > > error = EISDIR; > > goto bad; > > } > > ... > > thanks a lot for finding the problem in the src. what do you think of the > patch attached to this message? after applying it the example code i posted in > my previous message returns the following output (instead of EISDIR): > > rmdir errno: 16 (which is EBUSY) > mkdir errno: 17 (which is EEXIST) > > i don't know if these really are the correct return values, but it's what the > originator of the PR requested. > What if cn_nameiop is != LOOKUP but also neither DELETE nor CREATE, assuming that case is possible? I'd leave the original if-clause at the end to catch that. --- Gary Jennejohn _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Gary Jennejohn schrieb am 2009-11-06: > On Fri, 06 Nov 2009 16:32:22 +0100 (CET) > Alexander Best <alexbestms@...> wrote: > > Alex Dupre schrieb am 2009-11-06: > > > Alexander Best ha scritto: > > > > i dug up this old pr > > > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 > > > I think the EISDIR error is coming from kern/vfs_lookup.c, > > > lookup() > > > function with cn_nameptr = "": > > > /* > > > * Check for degenerate name (e.g. / or "") > > > * which is a way of talking about a directory, > > > * e.g. like "/." or ".". > > > */ > > > if (cnp->cn_nameptr[0] == '\0') { > > > ... > > > if (cnp->cn_nameiop != LOOKUP) { > > > error = EISDIR; > > > goto bad; > > > } > > > ... > > thanks a lot for finding the problem in the src. what do you think > > of the > > patch attached to this message? after applying it the example code > > i posted in > > my previous message returns the following output (instead of > > EISDIR): > > rmdir errno: 16 (which is EBUSY) > > mkdir errno: 17 (which is EEXIST) > > i don't know if these really are the correct return values, but > > it's what the > > originator of the PR requested. > What if cn_nameiop is != LOOKUP but also neither DELETE nor CREATE, > assuming that case is possible? I'd leave the original if-clause at > the end to catch that. > --- > Gary Jennejohn good point. cn_nameiop can be either LOOKUP, CREATE, RENAME, or DELETE. i'll check if maybe errno is also set incorrectly when RENAME is used and hack up a better patch which will handle all possible cn_nameiop values. thanks for pointing this out. alex _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Gary Jennejohn schrieb am 2009-11-06: > On Fri, 06 Nov 2009 16:32:22 +0100 (CET) > Alexander Best <alexbestms@...> wrote: > > Alex Dupre schrieb am 2009-11-06: > > > Alexander Best ha scritto: > > > > i dug up this old pr > > > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 > > > I think the EISDIR error is coming from kern/vfs_lookup.c, > > > lookup() > > > function with cn_nameptr = "": > > > /* > > > * Check for degenerate name (e.g. / or "") > > > * which is a way of talking about a directory, > > > * e.g. like "/." or ".". > > > / > > > if (cnp->cn_nameptr[0] == '\0') { > > > ... > > > if (cnp->cn_nameiop != LOOKUP) { > > > error = EISDIR; > > > goto bad; > > > } > > > ... > > thanks a lot for finding the problem in the src. what do you think > > of the > > patch attached to this message? after applying it the example code > > i posted in > > my previous message returns the following output (instead of > > EISDIR): > > rmdir errno: 16 (which is EBUSY) > > mkdir errno: 17 (which is EEXIST) > > i don't know if these really are the correct return values, but > > it's what the > > originator of the PR requested. > What if cn_nameiop is != LOOKUP but also neither DELETE nor CREATE, > assuming that case is possible? I'd leave the original if-clause at > the end to catch that. > --- > Gary Jennejohn how about this patch? 1. i've added "if (cnp->cn_nameiop != LOOKUP)" although i don't think it's necessary since the first blocks should cover all the possible cases. 2. i've used rename() to test the case (cnp->cn_nameiop != RENAME). is this correct or does rename() use a combo of DELETE and CREATE? problem is that the rename(2) manual doesn't seem to cover the case that arg 1 is a mountpoint. right now EBUSY gets returned if cnp->cn_nameiop != RENAME. however BUSY needs to be added to all manuals which use cnp->cn_nameiop != RENAME (shouldn't be too many). or are there any other suggestions what rename() should return if arg 1 is a mountpoint? cheers. alex --- vfs_lookup.c 2009-11-06 16:14:41.000000000 +0100 +++ /usr/src/sys/kern/vfs_lookup.c 2009-11-06 17:41:40.000000000 +0100 @@ -35,7 +35,7 @@ / #include <sys/cdefs.h> -__FBSDID("$FreeBSD$"); +__FBSDID("$FreeBSD: head/sys/kern/vfs_lookup.c 195939 2009-07-29 07:44:43Z rwatson $"); #include "opt_kdtrace.h" #include "opt_ktrace.h" @@ -563,6 +563,15 @@ error = ENOTDIR; goto bad; } + if (cnp->cn_nameiop == DELETE \|\| cnp->cn_nameiop == RENAME) { + error = EBUSY; + goto bad; + } + if (cnp->cn_nameiop == CREATE) { + error = EEXIST; + goto bad; + } + /* XXX This block might not be needed. */ if (cnp->cn_nameiop != LOOKUP) { error = EISDIR; goto bad; _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Gary Jennejohn-3 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Fri, 06 Nov 2009 17:43:06 +0100 (CET) Alexander Best <alexbestms@...> wrote: > Gary Jennejohn schrieb am 2009-11-06: > > On Fri, 06 Nov 2009 16:32:22 +0100 (CET) > > Alexander Best <alexbestms@...> wrote: > > > > Alex Dupre schrieb am 2009-11-06: > > > > Alexander Best ha scritto: > > > > > i dug up this old pr > > > > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 > > > > > I think the EISDIR error is coming from kern/vfs_lookup.c, > > > > lookup() > > > > function with cn_nameptr = "": > > > > > > /* > > > > * Check for degenerate name (e.g. / or "") > > > > * which is a way of talking about a directory, > > > > * e.g. like "/." or ".". > > > > */ > > > > if (cnp->cn_nameptr[0] == '\0') { > > > > ... > > > > if (cnp->cn_nameiop != LOOKUP) { > > > > error = EISDIR; > > > > goto bad; > > > > } > > > > ... > > > > thanks a lot for finding the problem in the src. what do you think > > > of the > > > patch attached to this message? after applying it the example code > > > i posted in > > > my previous message returns the following output (instead of > > > EISDIR): > > > > rmdir errno: 16 (which is EBUSY) > > > mkdir errno: 17 (which is EEXIST) > > > > i don't know if these really are the correct return values, but > > > it's what the > > > originator of the PR requested. > > > > What if cn_nameiop is != LOOKUP but also neither DELETE nor CREATE, > > assuming that case is possible? I'd leave the original if-clause at > > the end to catch that. > > > --- > > Gary Jennejohn > > how about this patch? > > 1. i've added "if (cnp->cn_nameiop != LOOKUP)" although i don't think it's > necessary since the first blocks should cover all the possible cases. > 2. i've used rename() to test the case (cnp->cn_nameiop != RENAME). is this > correct or does rename() use a combo of DELETE and CREATE? problem is that the > rename(2) manual doesn't seem to cover the case that arg 1 is a mountpoint. > right now EBUSY gets returned if cnp->cn_nameiop != RENAME. however BUSY needs > to be added to all manuals which use cnp->cn_nameiop != RENAME (shouldn't be > too many). or are there any other suggestions what rename() should return if > arg 1 is a mountpoint? > Hmm. In rename(2) there's [EINVAL] The from argument is a parent directory of to, or an attempt is made to rename `.' or `..'. and a few lines below your patch this case is handled for ISDOTDOT for both RENAME and DELETE. I don't see off hand where renaming or deleting "." is handled. According to the comment above your patch the case of "/." or "." is being checked, which would seem to correspond to the above part of rename(2), i.e. perhaps EINVAL should be returned for RENAME and DELETE. --- Gary Jennejohn _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Gary Jennejohn schrieb am 2009-11-06: > On Fri, 06 Nov 2009 17:43:06 +0100 (CET) > Alexander Best <alexbestms@...> wrote: > > Gary Jennejohn schrieb am 2009-11-06: > > > On Fri, 06 Nov 2009 16:32:22 +0100 (CET) > > > Alexander Best <alexbestms@...> wrote: > > > > Alex Dupre schrieb am 2009-11-06: > > > > > Alexander Best ha scritto: > > > > > > i dug up this old pr > > > > > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 > > > > > I think the EISDIR error is coming from kern/vfs_lookup.c, > > > > > lookup() > > > > > function with cn_nameptr = "": > > > > > /* > > > > > * Check for degenerate name (e.g. / or "") > > > > > * which is a way of talking about a directory, > > > > > * e.g. like "/." or ".". > > > > > */ > > > > > if (cnp->cn_nameptr[0] == '\0') { > > > > > ... > > > > > if (cnp->cn_nameiop != LOOKUP) { > > > > > error = EISDIR; > > > > > goto bad; > > > > > } > > > > > ... > > > > thanks a lot for finding the problem in the src. what do you > > > > think > > > > of the > > > > patch attached to this message? after applying it the example > > > > code > > > > i posted in > > > > my previous message returns the following output (instead of > > > > EISDIR): > > > > rmdir errno: 16 (which is EBUSY) > > > > mkdir errno: 17 (which is EEXIST) > > > > i don't know if these really are the correct return values, but > > > > it's what the > > > > originator of the PR requested. > > > What if cn_nameiop is != LOOKUP but also neither DELETE nor > > > CREATE, > > > assuming that case is possible? I'd leave the original if-clause > > > at > > > the end to catch that. > > > --- > > > Gary Jennejohn > > how about this patch? > > 1. i've added "if (cnp->cn_nameiop != LOOKUP)" although i don't > > think it's > > necessary since the first blocks should cover all the possible > > cases. > > 2. i've used rename() to test the case (cnp->cn_nameiop != RENAME). > > is this > > correct or does rename() use a combo of DELETE and CREATE? problem > > is that the > > rename(2) manual doesn't seem to cover the case that arg 1 is a > > mountpoint. > > right now EBUSY gets returned if cnp->cn_nameiop != RENAME. however > > BUSY needs > > to be added to all manuals which use cnp->cn_nameiop != RENAME > > (shouldn't be > > too many). or are there any other suggestions what rename() should > > return if > > arg 1 is a mountpoint? > Hmm. In rename(2) there's > [EINVAL] The from argument is a parent directory of to, or > an > attempt is made to rename `.' or `..'. > and a few lines below your patch this case is handled for ISDOTDOT > for both RENAME and DELETE. I don't see off hand where renaming or > deleting "." is handled. > According to the comment above your patch the case of "/." or "." is > being checked, which would seem to correspond to the above part of > rename(2), i.e. perhaps EINVAL should be returned for RENAME and > DELETE. > --- > Gary Jennejohn that would be an option. however in the case of rmdir(2) EINVAL and EBUYS would both fit. depends whether be forbid deletion of / because it is a mountpoint or because / is actually /. and paths ending with . are forbidden as arg in rmdir(2). i guess we have to take a look at the POSIX specs before we can decide how to handle this. also i've discovered that permission checks for / seem to be handled differently than any other dir. on my machine /usr is a mountpoint. doing rmdir /usr returns EACCES as regular user and EBUSY as superuser. doing rmdir / as regular user however doesn't seem to check permission but returns EBUSY right away. but that's not a problem i guess. this is probably happening because the kern/vfs_lookup.c code is being executed before anything else (including permission checks). i'll have a look what POSIX has to say about the return values. but i agree with you. returning EINVAL seems logical. alex. _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Alexander Best schrieb am 2009-11-06: > Gary Jennejohn schrieb am 2009-11-06: > > On Fri, 06 Nov 2009 17:43:06 +0100 (CET) > > Alexander Best <alexbestms@...> wrote: > > > Gary Jennejohn schrieb am 2009-11-06: > > > > On Fri, 06 Nov 2009 16:32:22 +0100 (CET) > > > > Alexander Best <alexbestms@...> wrote: > > > > > Alex Dupre schrieb am 2009-11-06: > > > > > > Alexander Best ha scritto: > > > > > > > i dug up this old pr > > > > > > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 > > > > > > I think the EISDIR error is coming from kern/vfs_lookup.c, > > > > > > lookup() > > > > > > function with cn_nameptr = "": > > > > > > /* > > > > > > * Check for degenerate name (e.g. / or "") > > > > > > * which is a way of talking about a directory, > > > > > > * e.g. like "/." or ".". > > > > > > */ > > > > > > if (cnp->cn_nameptr[0] == '\0') { > > > > > > ... > > > > > > if (cnp->cn_nameiop != LOOKUP) { > > > > > > error = EISDIR; > > > > > > goto bad; > > > > > > } > > > > > > ... > > > > > thanks a lot for finding the problem in the src. what do you > > > > > think > > > > > of the > > > > > patch attached to this message? after applying it the example > > > > > code > > > > > i posted in > > > > > my previous message returns the following output (instead of > > > > > EISDIR): > > > > > rmdir errno: 16 (which is EBUSY) > > > > > mkdir errno: 17 (which is EEXIST) > > > > > i don't know if these really are the correct return values, > > > > > but > > > > > it's what the > > > > > originator of the PR requested. > > > > What if cn_nameiop is != LOOKUP but also neither DELETE nor > > > > CREATE, > > > > assuming that case is possible? I'd leave the original > > > > if-clause > > > > at > > > > the end to catch that. > > > > --- > > > > Gary Jennejohn > > > how about this patch? > > > 1. i've added "if (cnp->cn_nameiop != LOOKUP)" although i don't > > > think it's > > > necessary since the first blocks should cover all the possible > > > cases. > > > 2. i've used rename() to test the case (cnp->cn_nameiop != > > > RENAME). > > > is this > > > correct or does rename() use a combo of DELETE and CREATE? > > > problem > > > is that the > > > rename(2) manual doesn't seem to cover the case that arg 1 is a > > > mountpoint. > > > right now EBUSY gets returned if cnp->cn_nameiop != RENAME. > > > however > > > BUSY needs > > > to be added to all manuals which use cnp->cn_nameiop != RENAME > > > (shouldn't be > > > too many). or are there any other suggestions what rename() > > > should > > > return if > > > arg 1 is a mountpoint? > > Hmm. In rename(2) there's > > [EINVAL] The from argument is a parent directory of to, > > or > > an > > attempt is made to rename `.' or `..'. > > and a few lines below your patch this case is handled for ISDOTDOT > > for both RENAME and DELETE. I don't see off hand where renaming or > > deleting "." is handled. > > According to the comment above your patch the case of "/." or "." > > is > > being checked, which would seem to correspond to the above part of > > rename(2), i.e. perhaps EINVAL should be returned for RENAME and > > DELETE. > > --- > > Gary Jennejohn > that would be an option. however in the case of rmdir(2) EINVAL and > EBUYS > would both fit. depends whether be forbid deletion of / because it is > a > mountpoint or because / is actually /. and paths ending with . are > forbidden > as arg in rmdir(2). > i guess we have to take a look at the POSIX specs before we can > decide how to > handle this. > also i've discovered that permission checks for / seem to be handled > differently than any other dir. on my machine /usr is a mountpoint. > doing > rmdir /usr returns EACCES as regular user and EBUSY as superuser. > doing rmdir > / as regular user however doesn't seem to check permission but > returns EBUSY > right away. but that's not a problem i guess. this is probably > happening > because the kern/vfs_lookup.c code is being executed before anything > else > (including permission checks). > i'll have a look what POSIX has to say about the return values. but i > agree > with you. returning EINVAL seems logical. > alex. ok. here it goes. POSIX says: rmdir() [http://www.opengroup.org/onlinepubs/9699919799/functions/rmdir.html#tag_16_618]: If the directory is the root directory or the current working directory of any process, it is unspecified whether the function succeeds, or whether it shall fail and set errno to [EBUSY]. mkdir() [http://www.opengroup.org/onlinepubs/9699919799/functions/mkdir.html#tag_16_372]: nothing regarding /. rename() [http://www.opengroup.org/onlinepubs/9699919799/functions/rename.html#tag_16_614]: If either pathname argument refers to a path whose final component is either dot or dot-dot, rename() shall fail. so at least rmdir() should return EBUSY. also POSIX defines EBUSY for return() which isn't documented in our return(2) manual. alex _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Alexander Best schrieb am 2009-11-06: > Alexander Best schrieb am 2009-11-06: > > Gary Jennejohn schrieb am 2009-11-06: > > > On Fri, 06 Nov 2009 17:43:06 +0100 (CET) > > > Alexander Best <alexbestms@...> wrote: > > > > Gary Jennejohn schrieb am 2009-11-06: > > > > > On Fri, 06 Nov 2009 16:32:22 +0100 (CET) > > > > > Alexander Best <alexbestms@...> wrote: > > > > > > Alex Dupre schrieb am 2009-11-06: > > > > > > > Alexander Best ha scritto: > > > > > > > > i dug up this old pr > > > > > > > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 > > > > > > > I think the EISDIR error is coming from > > > > > > > kern/vfs_lookup.c, > > > > > > > lookup() > > > > > > > function with cn_nameptr = "": > > > > > > > /* > > > > > > > * Check for degenerate name (e.g. / or "") > > > > > > > * which is a way of talking about a directory, > > > > > > > * e.g. like "/." or ".". > > > > > > > */ > > > > > > > if (cnp->cn_nameptr[0] == '\0') { > > > > > > > ... > > > > > > > if (cnp->cn_nameiop != LOOKUP) { > > > > > > > error = EISDIR; > > > > > > > goto bad; > > > > > > > } > > > > > > > ... > > > > > > thanks a lot for finding the problem in the src. what do > > > > > > you > > > > > > think > > > > > > of the > > > > > > patch attached to this message? after applying it the > > > > > > example > > > > > > code > > > > > > i posted in > > > > > > my previous message returns the following output (instead > > > > > > of > > > > > > EISDIR): > > > > > > rmdir errno: 16 (which is EBUSY) > > > > > > mkdir errno: 17 (which is EEXIST) > > > > > > i don't know if these really are the correct return values, > > > > > > but > > > > > > it's what the > > > > > > originator of the PR requested. > > > > > What if cn_nameiop is != LOOKUP but also neither DELETE nor > > > > > CREATE, > > > > > assuming that case is possible? I'd leave the original > > > > > if-clause > > > > > at > > > > > the end to catch that. > > > > > --- > > > > > Gary Jennejohn > > > > how about this patch? > > > > 1. i've added "if (cnp->cn_nameiop != LOOKUP)" although i don't > > > > think it's > > > > necessary since the first blocks should cover all the possible > > > > cases. > > > > 2. i've used rename() to test the case (cnp->cn_nameiop != > > > > RENAME). > > > > is this > > > > correct or does rename() use a combo of DELETE and CREATE? > > > > problem > > > > is that the > > > > rename(2) manual doesn't seem to cover the case that arg 1 is a > > > > mountpoint. > > > > right now EBUSY gets returned if cnp->cn_nameiop != RENAME. > > > > however > > > > BUSY needs > > > > to be added to all manuals which use cnp->cn_nameiop != RENAME > > > > (shouldn't be > > > > too many). or are there any other suggestions what rename() > > > > should > > > > return if > > > > arg 1 is a mountpoint? > > > Hmm. In rename(2) there's > > > [EINVAL] The from argument is a parent directory of to, > > > or > > > an > > > attempt is made to rename `.' or `..'. > > > and a few lines below your patch this case is handled for > > > ISDOTDOT > > > for both RENAME and DELETE. I don't see off hand where renaming > > > or > > > deleting "." is handled. > > > According to the comment above your patch the case of "/." or "." > > > is > > > being checked, which would seem to correspond to the above part > > > of > > > rename(2), i.e. perhaps EINVAL should be returned for RENAME and > > > DELETE. > > > --- > > > Gary Jennejohn > > that would be an option. however in the case of rmdir(2) EINVAL and > > EBUYS > > would both fit. depends whether be forbid deletion of / because it > > is > > a > > mountpoint or because / is actually /. and paths ending with . are > > forbidden > > as arg in rmdir(2). > > i guess we have to take a look at the POSIX specs before we can > > decide how to > > handle this. > > also i've discovered that permission checks for / seem to be > > handled > > differently than any other dir. on my machine /usr is a mountpoint. > > doing > > rmdir /usr returns EACCES as regular user and EBUSY as superuser. > > doing rmdir > > / as regular user however doesn't seem to check permission but > > returns EBUSY > > right away. but that's not a problem i guess. this is probably > > happening > > because the kern/vfs_lookup.c code is being executed before > > anything > > else > > (including permission checks). > > i'll have a look what POSIX has to say about the return values. but > > i > > agree > > with you. returning EINVAL seems logical. > > alex. > ok. here it goes. POSIX says: > rmdir() > [http://www.opengroup.org/onlinepubs/9699919799/functions/rmdir.html#tag_16_618]: > If the directory is the root directory or the current working > directory of any > process, it is unspecified whether the function succeeds, or whether > it shall > fail and set errno to [EBUSY]. > mkdir() > [http://www.opengroup.org/onlinepubs/9699919799/functions/mkdir.html#tag_16_372]: > nothing regarding /. > rename() > [http://www.opengroup.org/onlinepubs/9699919799/functions/rename.html#tag_16_614]: > If either pathname argument refers to a path whose final component is > either > dot or dot-dot, rename() shall fail. > so at least rmdir() should return EBUSY. also POSIX defines EBUSY for > return() > which isn't documented in our return(2) manual. > alex this is getting a bit more complicated now. i've had a look at kern/vfs_syscalls.c where the basic rmdir(), rename() and mkdir() cals are being defined. looks like they themself set certain errno values, but those values get overwritten by kern/vfs_lookup.c i guess the right approach would be this one: if rmdir(), rename() or mkdir() is called with / set errno=EISDIR in kern/vfs_lookup.c. then in kern/vfs_syscalls.c do something like if(errno == EISDIR) return XXX; XXX being EBUSY for rmdir(), EINVAL for rename(), etc. i'll see if i can get this sorted out. alex _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Gary Jennejohn schrieb am 2009-11-06: > On Fri, 06 Nov 2009 17:43:06 +0100 (CET) > Alexander Best <alexbestms@...> wrote: > > Gary Jennejohn schrieb am 2009-11-06: > > > On Fri, 06 Nov 2009 16:32:22 +0100 (CET) > > > Alexander Best <alexbestms@...> wrote: > > > > Alex Dupre schrieb am 2009-11-06: > > > > > Alexander Best ha scritto: > > > > > > i dug up this old pr > > > > > > http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/59739 > > > > > I think the EISDIR error is coming from kern/vfs_lookup.c, > > > > > lookup() > > > > > function with cn_nameptr = "": > > > > > /* > > > > > * Check for degenerate name (e.g. / or "") > > > > > * which is a way of talking about a directory, > > > > > * e.g. like "/." or ".". > > > > > / > > > > > if (cnp->cn_nameptr[0] == '\0') { > > > > > ... > > > > > if (cnp->cn_nameiop != LOOKUP) { > > > > > error = EISDIR; > > > > > goto bad; > > > > > } > > > > > ... > > > > thanks a lot for finding the problem in the src. what do you > > > > think > > > > of the > > > > patch attached to this message? after applying it the example > > > > code > > > > i posted in > > > > my previous message returns the following output (instead of > > > > EISDIR): > > > > rmdir errno: 16 (which is EBUSY) > > > > mkdir errno: 17 (which is EEXIST) > > > > i don't know if these really are the correct return values, but > > > > it's what the > > > > originator of the PR requested. > > > What if cn_nameiop is != LOOKUP but also neither DELETE nor > > > CREATE, > > > assuming that case is possible? I'd leave the original if-clause > > > at > > > the end to catch that. > > > --- > > > Gary Jennejohn > > how about this patch? > > 1. i've added "if (cnp->cn_nameiop != LOOKUP)" although i don't > > think it's > > necessary since the first blocks should cover all the possible > > cases. > > 2. i've used rename() to test the case (cnp->cn_nameiop != RENAME). > > is this > > correct or does rename() use a combo of DELETE and CREATE? problem > > is that the > > rename(2) manual doesn't seem to cover the case that arg 1 is a > > mountpoint. > > right now EBUSY gets returned if cnp->cn_nameiop != RENAME. however > > BUSY needs > > to be added to all manuals which use cnp->cn_nameiop != RENAME > > (shouldn't be > > too many). or are there any other suggestions what rename() should > > return if > > arg 1 is a mountpoint? > Hmm. In rename(2) there's > [EINVAL] The from argument is a parent directory of to, or > an > attempt is made to rename `.' or `..'. > and a few lines below your patch this case is handled for ISDOTDOT > for both RENAME and DELETE. I don't see off hand where renaming or > deleting "." is handled. > According to the comment above your patch the case of "/." or "." is > being checked, which would seem to correspond to the above part of > rename(2), i.e. perhaps EINVAL should be returned for RENAME and > DELETE. > --- > Gary Jennejohn here's a completely new patch. all the changes are in kern/vfs_syscall.c. so kern/vfs_lookup.c now stays just the way it is (please revert any changes from the previous patches i posted). after applying the patch this is the output from a slightly modified version of the test app i attached in my very first post: rmdir errno: 16 (EBUSY) <- EBUSY is required by POSIX mkdir errno: 17 (EEXIST) rename errno: 22 (EINVAL) these are the results when called with "/" as arg. the output doesn't differ if run with or without superuser privileges. when run with "." as arg this is the output as regular user: rmdir errno: 22 (EINVAL) mkdir errno: 17 (EEXIST) rename errno: 13 (EACCES) and as superuser: rmdir errno: 22 (EINVAL) mkdir errno: 17 (EEXIST) rename errno: 22 (EINVAL) does this look reasonable? would be nice if mkdir and rmdir would also check privileges at first like rename, but that's a different story i guess. ;) alex --- vfs_syscalls.c 2009-11-06 22:07:08.000000000 +0100 +++ /usr/src/sys/kern/vfs_syscalls.c 2009-11-06 21:37:47.000000000 +0100 @@ -35,7 +35,7 @@ / #include <sys/cdefs.h> -__FBSDID("$FreeBSD$"); +__FBSDID("$FreeBSD: head/sys/kern/vfs_syscalls.c 196887 2009-09-06 11:44:46Z kib $"); #include "opt_compat.h" #include "opt_kdtrace.h" @@ -3587,8 +3587,12 @@ AUDITVNODE1, pathseg, old, oldfd, td); #endif - if ((error = namei(&fromnd)) != 0) + if ((error = namei(&fromnd)) != 0) { + /* Translate error code for rename("/", "dir2"). / + if (error == EISDIR) + error = EINVAL; return (error); + } fvfslocked = NDHASGIANT(&fromnd); tvfslocked = 0; #ifdef MAC @@ -3737,8 +3741,12 @@ NDINIT_AT(&nd, CREATE, LOCKPARENT \| SAVENAME \| MPSAFE \| AUDITVNODE1, segflg, path, fd, td); nd.ni_cnd.cn_flags \|= WILLBEDIR; - if ((error = namei(&nd)) != 0) + if ((error = namei(&nd)) != 0) { + / Translate error code for mkdir("/"). / + if (error == EISDIR) + error = EEXIST; return (error); + } vfslocked = NDHASGIANT(&nd); vp = nd.ni_vp; if (vp != NULL) { @@ -3825,10 +3833,15 @@ bwillwrite(); NDINIT_AT(&nd, DELETE, LOCKPARENT \| LOCKLEAF \| MPSAFE \| AUDITVNODE1, pathseg, path, fd, td); - if ((error = namei(&nd)) != 0) + if ((error = namei(&nd)) != 0) { + / Translate error code for rmdir("/"). / + if (error == EISDIR) + error = EBUSY; return (error); + } vfslocked = NDHASGIANT(&nd); vp = nd.ni_vp; + / XXX namei() takes care of this case. / if (vp->v_type != VDIR) { error = ENOTDIR; goto out; @@ -3841,6 +3854,7 @@ goto out; } / + * XXX namei() takes care of this case. * The root of a mounted filesystem cannot be deleted. */ if (vp->v_vflag & VV_ROOT) { _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Gary Jennejohn-3 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Fri, 06 Nov 2009 22:09:49 +0100 (CET) Alexander Best <alexbestms@...> wrote: > here's a completely new patch. all the changes are in kern/vfs_syscall.c. so > kern/vfs_lookup.c now stays just the way it is (please revert any changes from > the previous patches i posted). > > after applying the patch this is the output from a slightly modified version > of the test app i attached in my very first post: > > rmdir errno: 16 (EBUSY) <- EBUSY is required by POSIX > mkdir errno: 17 (EEXIST) > rename errno: 22 (EINVAL) > > these are the results when called with "/" as arg. the output doesn't differ > if run with or without superuser privileges. > > when run with "." as arg this is the output as regular user: > > rmdir errno: 22 (EINVAL) > mkdir errno: 17 (EEXIST) > rename errno: 13 (EACCES) > > and as superuser: > > rmdir errno: 22 (EINVAL) > mkdir errno: 17 (EEXIST) > rename errno: 22 (EINVAL) > > does this look reasonable? would be nice if mkdir and rmdir would also check > privileges at first like rename, but that's a different story i guess. ;) > The only problem I see with this is that vfs_syscall.c now has knowledge about the internal details of vfs_lookup.c Maybe just mention in the comments that this is the case and could be a problem if vfs_lookup.c is changed for some reason? That's what I do when I have to use something like this. But we don't want to turn this into a gigantic bikeshed - we already have enough of those. --- Gary Jennejohn _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
	Re: rmdir(2) and mkdir(2) both return EISDIR for argument "/" by Alexander Best :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Gary Jennejohn schrieb am 2009-11-07: > On Fri, 06 Nov 2009 22:09:49 +0100 (CET) > Alexander Best <alexbestms@...> wrote: > > here's a completely new patch. all the changes are in > > kern/vfs_syscall.c. so > > kern/vfs_lookup.c now stays just the way it is (please revert any > > changes from > > the previous patches i posted). > > after applying the patch this is the output from a slightly > > modified version > > of the test app i attached in my very first post: > > rmdir errno: 16 (EBUSY) <- EBUSY is required by POSIX > > mkdir errno: 17 (EEXIST) > > rename errno: 22 (EINVAL) > > these are the results when called with "/" as arg. the output > > doesn't differ > > if run with or without superuser privileges. > > when run with "." as arg this is the output as regular user: > > rmdir errno: 22 (EINVAL) > > mkdir errno: 17 (EEXIST) > > rename errno: 13 (EACCES) > > and as superuser: > > rmdir errno: 22 (EINVAL) > > mkdir errno: 17 (EEXIST) > > rename errno: 22 (EINVAL) > > does this look reasonable? would be nice if mkdir and rmdir would > > also check > > privileges at first like rename, but that's a different story i > > guess. ;) > The only problem I see with this is that vfs_syscall.c now has > knowledge > about the internal details of vfs_lookup.c > Maybe just mention in the comments that this is the case and could be > a problem if vfs_lookup.c is changed for some reason? That's what I > do when I have to use something like this. > But we don't want to turn this into a gigantic bikeshed - we already > have enough of those. > --- > Gary Jennejohn well like you said there are 2 possibilities of dealing with this: 1. in vfs_lookup.c or 2. in vfs_syscall.c personally i think vfs_syscall.c is the better solution. i only tested rmdir(), rename() and mkdir(). however lots of other apps/functions might be depending on vfs_lookup.c and are expecting namei() to return EISDIR for arg="/". if we change this behaviour we might break a lot of other stuff. also looking at vfs_syscall.c shows that a lot of code in there already depends and knows about vfs_lookup.c internal details. so we wouldn't be changing the whole vfs_syscall.c semantics. but you're right. this is the perfect topic for a bikeshed. ;) i'll wait for a few more days and if nobody has a problem with the last patch i'll submitted it as followup to kern/59739. thanks a lot for your help. :) cheers. alex _______________________________________________ freebsd-hackers@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."