| Message ID | 20121127213127.GH27142@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Nov 27, 2012, at 4:31 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Mon, Nov 26, 2012 at 05:51:16PM -0500, J. Bruce Fields wrote: >> On Mon, Nov 26, 2012 at 05:38:49PM -0500, Chuck Lever wrote: >>> What exactly is the problem with the current code? >>> >>> client_resolve() can return a NULL in some cases. Why is it OK to >>> pass a NULL "ai" to client_compose() ? Looks like that can result in >>> a mountd segfault. >> >> Bah, I thought I'd checked this and found it was prepared to handle >> that, but no: >> >> client_check->check_wildcard() >> >> looks like it can oops. > > Right, so the real problem is just that we're skipping the downcall on > failure instead of responding with a negative cache entry. Thus we're > leaving the client hanging instead of returning an error. > > So, I suppose we should do the below, based on steved's suggestion > (compiled, otherwise untested). Thanks, this seems reasonable. > > --b. > > commit dfb31d861261c8461a2dc4fb7e8823f5169a9079 > Author: J. Bruce Fields <bfields@redhat.com> > Date: Tue Nov 27 16:10:41 2012 -0500 > > mountd: auth_unix_ip should downcall on error to prevent hangs > > Since bf6a4febaa78bf188896b7b5b02c46562dd08b70 "mountd: handle > allocation failures in auth_unix_ip upcall", a failure to map the > address of an incoming client to a name could result in a hang. > > We should be responding with an error in the case, not just skipping the > downcall and leaving everybody hanging. > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c > index e950ec6..c13f305 100644 > --- a/utils/mountd/cache.c > +++ b/utils/mountd/cache.c > @@ -109,12 +109,10 @@ static void auth_unix_ip(FILE *f) > struct addrinfo *ai = NULL; > > ai = client_resolve(tmp->ai_addr); > - if (ai == NULL) > - goto out; > - client = client_compose(ai); > - freeaddrinfo(ai); > - if (!client) > - goto out; > + if (ai) { > + client = client_compose(ai); > + freeaddrinfo(ai); > + } > } > qword_print(f, "nfsd"); > qword_print(f, ipaddr); > @@ -127,7 +125,6 @@ static void auth_unix_ip(FILE *f) > xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT"); > > free(client); > -out: > freeaddrinfo(tmp); > > } -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/11/12 16:31, J. Bruce Fields wrote: > On Mon, Nov 26, 2012 at 05:51:16PM -0500, J. Bruce Fields wrote: > commit dfb31d861261c8461a2dc4fb7e8823f5169a9079 > Author: J. Bruce Fields <bfields@redhat.com> > Date: Tue Nov 27 16:10:41 2012 -0500 > > mountd: auth_unix_ip should downcall on error to prevent hangs > > Since bf6a4febaa78bf188896b7b5b02c46562dd08b70 "mountd: handle > allocation failures in auth_unix_ip upcall", a failure to map the > address of an incoming client to a name could result in a hang. > > We should be responding with an error in the case, not just skipping the > downcall and leaving everybody hanging. > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> Committed.... steved. > > diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c > index e950ec6..c13f305 100644 > --- a/utils/mountd/cache.c > +++ b/utils/mountd/cache.c > @@ -109,12 +109,10 @@ static void auth_unix_ip(FILE *f) > struct addrinfo *ai = NULL; > > ai = client_resolve(tmp->ai_addr); > - if (ai == NULL) > - goto out; > - client = client_compose(ai); > - freeaddrinfo(ai); > - if (!client) > - goto out; > + if (ai) { > + client = client_compose(ai); > + freeaddrinfo(ai); > + } > } > qword_print(f, "nfsd"); > qword_print(f, ipaddr); > @@ -127,7 +125,6 @@ static void auth_unix_ip(FILE *f) > xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT"); > > free(client); > -out: > freeaddrinfo(tmp); > > } > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c index e950ec6..c13f305 100644 --- a/utils/mountd/cache.c +++ b/utils/mountd/cache.c @@ -109,12 +109,10 @@ static void auth_unix_ip(FILE *f) struct addrinfo *ai = NULL; ai = client_resolve(tmp->ai_addr); - if (ai == NULL) - goto out; - client = client_compose(ai); - freeaddrinfo(ai); - if (!client) - goto out; + if (ai) { + client = client_compose(ai); + freeaddrinfo(ai); + } } qword_print(f, "nfsd"); qword_print(f, ipaddr); @@ -127,7 +125,6 @@ static void auth_unix_ip(FILE *f) xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT"); free(client); -out: freeaddrinfo(tmp); }