Re: [Xen-devel] [PATCH v4 00/12] xenstore: support reading directory with many children

[Juergen Gross <jgross@xxxxxxxx>]

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