Xen devel - Sep 2007 - Xenstored: eliminate double free and fixes for memory leak

Please review.   This patch fixes a few problems:

a.   In handle_input(), which is called from main() in
xenstored_core.c, the conn->in is freed when there is no error, but
conn is freed when there is error.   This is inconsistent.  Moreover,
the conn is freed again upon exit from handle_input(), inside the
main.

b.   Another problem is that perms_to_strings() returned a allocated
memory (via realloc()).   The only time the value is returned is in
do_get_perms(), but immediately after send_reply(), the pointer is
immediately thrown away, resulting in a memory leak condition, as
send_reply() does not free any memory.

c.   Many of the functions like read_node(), get_parent(),
talloc_asprintf() etc all will allocate memory and return a pointer
when memory is allocated, but many times these are not freed.

Another problem identified but which I cannot fix, is that
create_hashtable() can return NULL in low memory condition.   But this
is not checked in the recursive function check_store_().   There is no
value to specify error or not in the return value of check_store_()
(declared as void).   May be we should change this to unsigned int or
something like that, so that error condition can be returned?

I am still making changes to the same file, but was hesitant to
incorporate so many changes as it will make the review more difficult.

I will submit these further changes in the next posting.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The changes are too interdependent, and therefore, I am now
resubmitting it as a complete patch, instead of splitting it up into
multiple patches.   All goes to the same file - xenstored_core.c.

On top of the comments given above, here are the additional comments:

Two FIXME comments added - for which a problem has been identified,
but fixing it will incurred a lot of changes in design etc.

First problem is the returned value from get_parent() - it can be
either a static value, or a dynamically allocated value (from
talloc_asprintf()), and therefore it is very difficult to judge
whether to free the memory or not.

Second (minor) problem is during destroy_node(), the allocated memory
within the node structure is not free.   These are allocated in
construct_node() function.

Please comments, thanks.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Apologized, the patch file was generated in reverse :-).


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On top of abovementioned problems I missed out this one:

canonicalize() may return an existing pointer as a value - so no
memory deallocation is needed, but it also may return a newly
allocated memory - for which deallocation is needed.   And currently
all the results returned from canonicalize() are not freed.

Please let me know how to fix this.

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

I don''t believe there are any memory leaks in xenstored -- talloc()
works
somewhat differently than malloc() in that each allocation can specify a
parent ''context''. This creates a hierarchy of allocated memory
blocks: any
memory block freed also automatically frees its children. When making a
talloc*() call, the first argument is usually the parent context or parent
allocation.

So, in the case of canonicalize(), either it returns the passed-in node, or
it returns a new string as a child of node. In the latter case, when the
original node string is freed, any string allocated by canonicalize() will
also be freed. If you trace any of your other leaks you should find that the
memory is actually freed, probably by the talloc_free() in
consider_message(). Take a look at xenstore/talloc_guide.txt for more info
about talloc.

As for not checking allocations for returning NULL, this is consistently not
done throughout xenstored. Mainly because there isn''t much we can do in
that
case, except perhaps log the error and then exit (which basically takes out
the host, as xenstored cannot be restarted). Rather than adding NULL checks
throughout xenstored, it would be acceptable to add a NULL check in
_talloc() which then logs and exits ''cleanly''.

 -- Keir


On 17/9/07 08:07, "Peter Teoh" <htmldeveloper@gmail.com> wrote:
> On top of abovementioned problems I missed out this one:
> 
> canonicalize() may return an existing pointer as a value - so no
> memory deallocation is needed, but it also may return a newly
> allocated memory - for which deallocation is needed.   And currently
> all the results returned from canonicalize() are not freed.
> 
> Please let me know how to fix this.
> 
> Thanks.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel