[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 00/12] xenstore: support reading directory with many children
On 05/12/16 19:19, Andrew Cooper wrote: > On 05/12/16 12:05, Wei Liu wrote: >> On Mon, Dec 05, 2016 at 08:48:41AM +0100, Juergen Gross wrote: >> [...] >>> Juergen Gross (12): >>> xenstore: modify add_change_node() parameter types >>> xenstore: call add_change_node() directly when writing node >>> xenstore: use common tdb record header in xenstore >>> xenstore: add per-node generation counter >>> xenstore: add support for reading directory with many children >>> xenstore: support XS_DIRECTORY_PART in libxenstore >>> xenstore: use array for xenstore wire command handling >>> xenstore: let command functions return error or success >>> xenstore: make functions static >>> xenstore: add helper functions for wire argument parsing >>> xenstore: add small default data buffer to internal struct >>> xenstore: handle memory allocation failures in xenstored >>> >> Applied to staging. > > XenServer's Coverity has run, and has a few things to say. Its not > obvious (i.e. I can't trivially identify) if these are preexisting bugs > which your code has brought to light, or introduced by your series. > > Both do_rm() and do_mkdir() suffer from the same problem. > > onearg(in) may return NULL, which results in get_node_canonicalized() > setting name to NULL and returning NULL. name is then dereferenced in > the error path by get_parent()/create_node() respectively. No. errno will be EINVAL and this will prohibit to enter the said paths guarded by errno == ENOENT. > There are two complains of leaking a talloc_array() allocation. First > in do_set_perms() via the xs_strings_to_perms() failure path and second No. The allocation context is "node" which is allocated using "in" as allocation context. This is freed when the command has been processed. > in send_directory_part() via the success path. The use of No again. Allocation context is "in" to be freed after processing the command. > talloc_array() in add_event() would suggest that you are expected to > call talloc_free() on the result, unless I am missing something subtle. You do. The free in add_event() is done as the allocation context might be NULL (e.g. when called from domain_cleanup()). So here the free is really needed in order to avoid a leak. > There is another resource leak complaint of bdata in send_reply(), > although this case clearly hasn't observed the list_add_tail(), so I > don't think this is a real issue. Right. > Finally, xs_directory_part(), on line 619 uses a strncpy() for all bytes > of gen, but doesn't necesserily NULL terminate it, and is later used by > strcmp(). This is a real issue. I'll send a patch. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |