I've got a customer experiencing a small memory leak in rpc.mountd using
nfs-utils-1.0.7. For their load it's about 150 KB/day. I have been
previously pointed me to a memory leak in add_name in
support/export/client.c. Thanks! they were seeing a 6 MB/ day leak
before that fix.
I've had trouble reproducing the same leak here, but in looking through
the code I believe I have come across another possible memory leak in
the support/export/client.c file. This time in the client_compose()
routine. It may be the source of the current 150 KB/day leak my customer
is seeing.
I've attached an anlysis from code inspection, but it boils down to the
fact that the 'he' variable of client_compose() needs to be xfree'd
before client_compose() returns. The 'he' variable is assigned a
xmalloc'ed value via either get_hostent() or get_reliable_hostbyaddr().
Once the client_compose() routine returns the pointer will be lost
forever. The code is identical in nfs-utils-1.1.0 so I believe the
memory leak exists there as well.
I'll keep trying to verify that indeed there is a memory leak here, but
from code inspection it certainly seems like it.
Let me know if you agree that client_compose() has a memory leak.
Thanks
Chris Kottaridis
Senior Engineer
Wind River Systems
719-522-9786
On Tue, 7 Aug 2007 14:27:39 -0700
"Kottaridis, Chris" <[email protected]> wrote:
> I've got a customer experiencing a small memory leak in rpc.mountd using
> nfs-utils-1.0.7. For their load it's about 150 KB/day. I have been
> previously pointed me to a memory leak in add_name in
> support/export/client.c. Thanks! they were seeing a 6 MB/ day leak
> before that fix.
>
> I've had trouble reproducing the same leak here, but in looking through
> the code I believe I have come across another possible memory leak in
> the support/export/client.c file. This time in the client_compose()
> routine. It may be the source of the current 150 KB/day leak my customer
> is seeing.
>
> I've attached an anlysis from code inspection, but it boils down to the
> fact that the 'he' variable of client_compose() needs to be xfree'd
> before client_compose() returns. The 'he' variable is assigned a
> xmalloc'ed value via either get_hostent() or get_reliable_hostbyaddr().
> Once the client_compose() routine returns the pointer will be lost
> forever. The code is identical in nfs-utils-1.1.0 so I believe the
> memory leak exists there as well.
>
> I'll keep trying to verify that indeed there is a memory leak here, but
> from code inspection it certainly seems like it.
>
> Let me know if you agree that client_compose() has a memory leak.
>
Current nfs-utils git tree seems to do this already. Looks like this
patch fixed it:
commit 1cb4a250fb9f0a8ba34befa47d951430e444a58e
Author: Steinar H. Gunderson <[email protected]>
Date: Fri May 11 21:02:09 2007 +1000
Memory leak in mountd
In client_compose(), free() the hostent structure returned before
exiting. Normally, gethostbyaddr() returns a pointer to a static
struct, but this hostent comes from either get_reliable_hostbyaddr() or
get_hostent(), both which return a pointer they privately xmalloc()ed,
which thus can and should be free()d.
Signed-Off-By: Steinar H. Gunderson <[email protected]>
You might want to test with that code and see if the problem
goes away...
Cheers,
--
Jeff Layton <[email protected]>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Thanks.
Also, it seems possible for get_hostent() to return a NULL value if the
malloc fails. The client_check() doesn't perform any kind of sanity
check on it's second parameter, the 'he' variable of client_compose().
So, there is the possibility that client_check() can try and derefence a
NULL pointer.
In other locations in the code where get_reliable_hostbyaddr() or
get_hostent() is called a sanity check for NULL is done to make sure the
value isn't NULL. For example in auth_authenticate():
hp = get_reliable_hostbyaddr((const char*)&caller->sin_addr,
sizeof(struct in_addr),
AF_INET);
if (!hp)
hp = get_hostent((const char*)&caller->sin_addr,
sizeof(struct in_addr),
AF_INET);
if (!hp)
return exp;
Should there be a similar check on 'he' in client_compose to prevent a
NULL pointer dereference in client_check() ? Something like:
if (clientlist[MCL_WILDCARD] || clientlist[MCL_NETGROUP])
he = get_reliable_hostbyaddr((const char*)&addr,
sizeof(addr), AF_INET);
if (he == NULL)
he = get_hostent((const char*)&addr, sizeof(addr),
AF_INET);
if (he == NULL)
return name;
Seems like a more graceful way to handle a malloc failure in
get_hostent().
Thanks
Chris Kottaridis
Senior Engineer
Wind River Systems
719-522-9786
-----Original Message-----
From: Jeff Layton [mailto:[email protected]]
Sent: Tuesday, August 07, 2007 6:22 PM
To: [email protected]
Cc: Kottaridis, Chris
Subject: Re: [NFS] rpc.mountd memory leak
On Tue, 7 Aug 2007 14:27:39 -0700
"Kottaridis, Chris" <[email protected]> wrote:
> I've got a customer experiencing a small memory leak in rpc.mountd
> using nfs-utils-1.0.7. For their load it's about 150 KB/day. I have
> been previously pointed me to a memory leak in add_name in
> support/export/client.c. Thanks! they were seeing a 6 MB/ day leak
> before that fix.
>
> I've had trouble reproducing the same leak here, but in looking
> through the code I believe I have come across another possible memory
> leak in the support/export/client.c file. This time in the
> client_compose() routine. It may be the source of the current 150
> KB/day leak my customer is seeing.
>
> I've attached an anlysis from code inspection, but it boils down to
> the fact that the 'he' variable of client_compose() needs to be
> xfree'd before client_compose() returns. The 'he' variable is assigned
> a xmalloc'ed value via either get_hostent() or
get_reliable_hostbyaddr().
> Once the client_compose() routine returns the pointer will be lost
> forever. The code is identical in nfs-utils-1.1.0 so I believe the
> memory leak exists there as well.
>
> I'll keep trying to verify that indeed there is a memory leak here,
> but from code inspection it certainly seems like it.
>
> Let me know if you agree that client_compose() has a memory leak.
>
Current nfs-utils git tree seems to do this already. Looks like this
patch fixed it:
commit 1cb4a250fb9f0a8ba34befa47d951430e444a58e
Author: Steinar H. Gunderson <[email protected]>
Date: Fri May 11 21:02:09 2007 +1000
Memory leak in mountd
In client_compose(), free() the hostent structure returned before
exiting. Normally, gethostbyaddr() returns a pointer to a static
struct, but this hostent comes from either get_reliable_hostbyaddr()
or
get_hostent(), both which return a pointer they privately
xmalloc()ed,
which thus can and should be free()d.
Signed-Off-By: Steinar H. Gunderson <[email protected]>
You might want to test with that code and see if the problem goes
away...
Cheers,
--
Jeff Layton <[email protected]>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Wed, 8 Aug 2007 13:13:51 -0700
"Kottaridis, Chris" <[email protected]> wrote:
> Thanks.
>
> Also, it seems possible for get_hostent() to return a NULL value if the
> malloc fails. The client_check() doesn't perform any kind of sanity
> check on it's second parameter, the 'he' variable of client_compose().
> So, there is the possibility that client_check() can try and derefence a
> NULL pointer.
>
> In other locations in the code where get_reliable_hostbyaddr() or
> get_hostent() is called a sanity check for NULL is done to make sure the
> value isn't NULL. For example in auth_authenticate():
>
> hp = get_reliable_hostbyaddr((const char*)&caller->sin_addr,
> sizeof(struct in_addr),
> AF_INET);
> if (!hp)
> hp = get_hostent((const char*)&caller->sin_addr,
> sizeof(struct in_addr),
> AF_INET);
> if (!hp)
> return exp;
>
> Should there be a similar check on 'he' in client_compose to prevent a
> NULL pointer dereference in client_check() ? Something like:
>
> if (clientlist[MCL_WILDCARD] || clientlist[MCL_NETGROUP])
> he = get_reliable_hostbyaddr((const char*)&addr,
> sizeof(addr), AF_INET);
> if (he == NULL)
> he = get_hostent((const char*)&addr, sizeof(addr),
> AF_INET);
>
> if (he == NULL)
> return name;
>
> Seems like a more graceful way to handle a malloc failure in
> get_hostent().
>
> Thanks
>
get_hostent() uses xmalloc() which should make the program die if it
fails. So checking for he == NULL really isn't needed.
--
Jeff Layton <[email protected]>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
>get_hostent() uses xmalloc() which should make the program die if it
fails. So checking for
>he == NULL really isn't needed.
True in 1.1.0, I should have looked there first! Not in 1.0.7.
Again Thanks for the info
Chris Kottaridis
Senior Engineer
Wind River Systems
719-522-9786
-----Original Message-----
From: Jeff Layton [mailto:[email protected]]
Sent: Wednesday, August 08, 2007 2:20 PM
To: Kottaridis, Chris
Cc: [email protected]
Subject: Re: [NFS] rpc.mountd memory leak
On Wed, 8 Aug 2007 13:13:51 -0700
"Kottaridis, Chris" <[email protected]> wrote:
> Thanks.
>
> Also, it seems possible for get_hostent() to return a NULL value if
> the malloc fails. The client_check() doesn't perform any kind of
> sanity check on it's second parameter, the 'he' variable of
client_compose().
> So, there is the possibility that client_check() can try and derefence
> a NULL pointer.
>
> In other locations in the code where get_reliable_hostbyaddr() or
> get_hostent() is called a sanity check for NULL is done to make sure
> the value isn't NULL. For example in auth_authenticate():
>
> hp = get_reliable_hostbyaddr((const char*)&caller->sin_addr,
> sizeof(struct in_addr),
> AF_INET);
> if (!hp)
> hp = get_hostent((const char*)&caller->sin_addr,
> sizeof(struct in_addr),
> AF_INET);
> if (!hp)
> return exp;
>
> Should there be a similar check on 'he' in client_compose to prevent a
> NULL pointer dereference in client_check() ? Something like:
>
> if (clientlist[MCL_WILDCARD] || clientlist[MCL_NETGROUP])
> he = get_reliable_hostbyaddr((const char*)&addr,
> sizeof(addr), AF_INET);
> if (he == NULL)
> he = get_hostent((const char*)&addr, sizeof(addr),
> AF_INET);
>
> if (he == NULL)
> return name;
>
> Seems like a more graceful way to handle a malloc failure in
> get_hostent().
>
> Thanks
>
get_hostent() uses xmalloc() which should make the program die if it
fails. So checking for he == NULL really isn't needed.
--
Jeff Layton <[email protected]>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs