On 08/ 3/10 03:24 AM, Neil Brown wrote:
Hi Chuck,
I've been reviewing some recent nfs-utils commits prior to possibly
including them in a suse product.
I think there is part of
commit fbc1b2c8e47ee9371c3565b266f1386575b46920
Author: Chuck Lever<chuck.lever@xxxxxxxxxx>
Date: Tue Jun 22 12:43:01 2010 -0400
mountd: Replace "struct hostent" with "struct addrinfo"
which is wrong. However I'm not sure if it actually breaks something or if
the relevant code is effectively dead.
The questionable chunk is in check_netgroup
/* If hname is ip address convert to FQDN */
- if (inet_aton(hname,&addr.sin_addr)&&
- (nhp = gethostbyaddr((const char *)&(addr.sin_addr),
- sizeof(addr.sin_addr), AF_INET))) {
- hname = nhp->h_name;
+ tmp = host_pton(hname);
+ if (tmp != NULL) {
+ freeaddrinfo(tmp);
if (innetgr(netgroup, hname, NULL, NULL))
return 1;
}
The new code is effectively a no-op - it conditionally calls
if (innetgr(netgroup, hname, NULL, NULL))
return 1;
and if that succeeds, it would have already succeeded near the top of the
function so we would not get here.
The original code checked if 'hname' looked like an IP address. If it did it
looked up the canonical name and checked that for membership in the netgroup.
I'm not entirely sure when check_netgroup (or client_check) would be called
with such a string in a context where doing a lookup would make sense. I
hunted through the code and found some rmtab manipulations that could result
in this, but I'm not sure.
This code was originally added about 9 years ago in what is now commit
86ae664e66c439354cb4f959e9f289059e7760a4
which blames it on Anne Milicia<milicia@xxxxxxxxxxxxxxxxxxxxxxxx> without
any real explanation. I don't suppose Anne is still around ???
Did you think about this at all when you made the patch? Do you have any
thoughts about whether that section of code is still needed? I don't feel
very comfortable removing it without knowing why it is there, and I don't
really know why it is there....
I'm not really able to exercise the netgroup logic here, since I don't
use netgroups. I assumed that since Steve touched this code as recently
as last year (see commit 6a72b8af), he would have some test cases to
ensure I wasn't breaking anything badly. He might even know of real
customer scenarios that depend on this logic.
Anyway, from a local static analysis standpoint, it looks like removing
the gethostbyaddr(3) call is a bug. The code should continue to perform
a reverse lookup there to get a new name to use with the second
innetgr() call.
I recommend this: using the new calls I added, the thing to do is pass
tmp->ai_addr to host_canonname(), and then call freeaddrinfo(tmp). The
string returned by host_canonname() can then be passed to the second
innetgr(3) call, but needs to be freed before check_netgroup() can
return to its caller. That would allow somewhat more confidence that
the semantics of this function don't change. If you like, I can code
something and post it.
Thanks much for the careful review.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
