| Message ID | 20210322210238.96915-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exportfs: fix unexporting of '/' | expand |
On Mon, Mar 22, 2021 at 10:02:38PM +0100, Ondrej Mosnacek wrote: > The code that has been added to strip trailing slashes from path in > unexportfs_parsed() forgot to account for the case of the root > directory, which is simply '/'. In that case it accesses path[-1] and > reduces the path to an empty string, which then fails to match any > export. > > Fix it by stopping the stripping when the path is just a single > character - it doesn't matter if it's a '/' or not, we want to keep it > either way in that case. Makes sense to me. (Note nfs-exporting / is often a bad idea. I assume you had some good reason in this case.) --b. > > Reproducer: > > exportfs localhost:/ > exportfs -u localhost:/ > > Without this patch, the unexport step fails with "exportfs: Could not > find 'localhost:/' to unexport." > > Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()") > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171 > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > utils/exportfs/exportfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c > index 262dd19a..1aedd3d6 100644 > --- a/utils/exportfs/exportfs.c > +++ b/utils/exportfs/exportfs.c > @@ -383,7 +383,7 @@ unexportfs_parsed(char *hname, char *path, int verbose) > * so need to deal with it. > */ > size_t nlen = strlen(path); > - while (path[nlen - 1] == '/') > + while (nlen > 1 && path[nlen - 1] == '/') > nlen--; > > for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) { > -- > 2.30.2
On Thu, Mar 25, 2021 at 8:27 PM J. Bruce Fields <bfields@fieldses.org> wrote: > On Mon, Mar 22, 2021 at 10:02:38PM +0100, Ondrej Mosnacek wrote: > > The code that has been added to strip trailing slashes from path in > > unexportfs_parsed() forgot to account for the case of the root > > directory, which is simply '/'. In that case it accesses path[-1] and > > reduces the path to an empty string, which then fails to match any > > export. > > > > Fix it by stopping the stripping when the path is just a single > > character - it doesn't matter if it's a '/' or not, we want to keep it > > either way in that case. > > Makes sense to me. > > (Note nfs-exporting / is often a bad idea. I assume you had some good > reason in this case.) I hit it in a test, so if the only issue is that it exposes more than necessary, then I guess it's fine in this case :) > > --b. > > > > > Reproducer: > > > > exportfs localhost:/ > > exportfs -u localhost:/ > > > > Without this patch, the unexport step fails with "exportfs: Could not > > find 'localhost:/' to unexport." > > > > Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()") > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171 > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > utils/exportfs/exportfs.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c > > index 262dd19a..1aedd3d6 100644 > > --- a/utils/exportfs/exportfs.c > > +++ b/utils/exportfs/exportfs.c > > @@ -383,7 +383,7 @@ unexportfs_parsed(char *hname, char *path, int verbose) > > * so need to deal with it. > > */ > > size_t nlen = strlen(path); > > - while (path[nlen - 1] == '/') > > + while (nlen > 1 && path[nlen - 1] == '/') > > nlen--; > > > > for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) { > > -- > > 2.30.2 > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Thu, Mar 25, 2021 at 08:48:57PM +0100, Ondrej Mosnacek wrote: > On Thu, Mar 25, 2021 at 8:27 PM J. Bruce Fields <bfields@fieldses.org> wrote: > > On Mon, Mar 22, 2021 at 10:02:38PM +0100, Ondrej Mosnacek wrote: > > > The code that has been added to strip trailing slashes from path in > > > unexportfs_parsed() forgot to account for the case of the root > > > directory, which is simply '/'. In that case it accesses path[-1] and > > > reduces the path to an empty string, which then fails to match any > > > export. > > > > > > Fix it by stopping the stripping when the path is just a single > > > character - it doesn't matter if it's a '/' or not, we want to keep it > > > either way in that case. > > > > Makes sense to me. > > > > (Note nfs-exporting / is often a bad idea. I assume you had some good > > reason in this case.) > > I hit it in a test, so if the only issue is that it exposes more than > necessary, then I guess it's fine in this case :) Yes, in a lot of typical setups, exporting "/" would expose files to the network that you don't really want to. But, anyway, it should work. Looks like a good fix. --b. > > > > > --b. > > > > > > > > Reproducer: > > > > > > exportfs localhost:/ > > > exportfs -u localhost:/ > > > > > > Without this patch, the unexport step fails with "exportfs: Could not > > > find 'localhost:/' to unexport." > > > > > > Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()") > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171 > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > > --- > > > utils/exportfs/exportfs.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c > > > index 262dd19a..1aedd3d6 100644 > > > --- a/utils/exportfs/exportfs.c > > > +++ b/utils/exportfs/exportfs.c > > > @@ -383,7 +383,7 @@ unexportfs_parsed(char *hname, char *path, int verbose) > > > * so need to deal with it. > > > */ > > > size_t nlen = strlen(path); > > > - while (path[nlen - 1] == '/') > > > + while (nlen > 1 && path[nlen - 1] == '/') > > > nlen--; > > > > > > for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) { > > > -- > > > 2.30.2 > > > > > -- > Ondrej Mosnacek > Software Engineer, Linux Security - SELinux kernel > Red Hat, Inc.
On 3/22/21 5:02 PM, Ondrej Mosnacek wrote: > The code that has been added to strip trailing slashes from path in > unexportfs_parsed() forgot to account for the case of the root > directory, which is simply '/'. In that case it accesses path[-1] and > reduces the path to an empty string, which then fails to match any > export. > > Fix it by stopping the stripping when the path is just a single > character - it doesn't matter if it's a '/' or not, we want to keep it > either way in that case. > > Reproducer: > > exportfs localhost:/ > exportfs -u localhost:/ > > Without this patch, the unexport step fails with "exportfs: Could not > find 'localhost:/' to unexport." > > Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()") > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171 > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Committed... (tag: nfs-utils-2-5-4-rc2) steved. > --- > utils/exportfs/exportfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c > index 262dd19a..1aedd3d6 100644 > --- a/utils/exportfs/exportfs.c > +++ b/utils/exportfs/exportfs.c > @@ -383,7 +383,7 @@ unexportfs_parsed(char *hname, char *path, int verbose) > * so need to deal with it. > */ > size_t nlen = strlen(path); > - while (path[nlen - 1] == '/') > + while (nlen > 1 && path[nlen - 1] == '/') > nlen--; > > for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) { >
diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c index 262dd19a..1aedd3d6 100644 --- a/utils/exportfs/exportfs.c +++ b/utils/exportfs/exportfs.c @@ -383,7 +383,7 @@ unexportfs_parsed(char *hname, char *path, int verbose) * so need to deal with it. */ size_t nlen = strlen(path); - while (path[nlen - 1] == '/') + while (nlen > 1 && path[nlen - 1] == '/') nlen--; for (exp = exportlist[htype].p_head; exp; exp = exp->m_next) {
The code that has been added to strip trailing slashes from path in unexportfs_parsed() forgot to account for the case of the root directory, which is simply '/'. In that case it accesses path[-1] and reduces the path to an empty string, which then fails to match any export. Fix it by stopping the stripping when the path is just a single character - it doesn't matter if it's a '/' or not, we want to keep it either way in that case. Reproducer: exportfs localhost:/ exportfs -u localhost:/ Without this patch, the unexport step fails with "exportfs: Could not find 'localhost:/' to unexport." Fixes: a9a7728d8743 ("exportfs: Deal with path's trailing "/" in unexportfs_parsed()") Link: https://bugzilla.redhat.com/show_bug.cgi?id=1941171 Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- utils/exportfs/exportfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)