[LEDE-DEV] ubus: ubusd segfault on lookup

[Felix Fietkau]

On 2016-08-23 12:38, Delio Brignoli wrote:
> Hi Felix,
> 
>> On 23 Aug 2016, at 11:40, Felix Fietkau <nbd at nbd.name> wrote:
>> 
>> On 2016-08-18 12:19, Delio Brignoli wrote:
>>> Hello Felix,
>>> 
>>> ubusd_handle_remove_object() currently sends object removal event
>>> followed by a reply to the peer’s remove object request. However the
>>> payload of the reply is the same as the object removal event. This
>>> results in the peer not clearing the type id in struct ubus_object_type
>>> because UBUS_ATTR_OBJTYPE is missing form the reply. If the peer
>>> attempts to add the object again, the object will end up having a NULL
>>> type field and a lookup will trigger a NULL pointer dereference in
>>> used_prot.c:131
>>> 
>>> Swapping the order of ubus_proto_send_msg_from_blob() and
>>> ubusd_free_object() (see diff below) the peer sees the expected
>>> messages. Is this an acceptable fix for the issue? It looks like
>>> ubusd_create_object() allows object with NULL type to be created,
>>> probably because they are used internally, but shouldn’t it reply with
>>> an error when a peer tries to add an object with a non-existent type id?
>>> 
>>> Thank you
>>> —
>>> Delio
>>> 
>>> 
>>> diff --git a/ubusd_proto.c b/ubusd_proto.c
>>> index 0af11f2..d0cfa10 100644
>>> --- a/ubusd_proto.c
>>> +++ b/ubusd_proto.c
>>> @@ -130,8 +130,8 @@ static int ubusd_handle_remove_object(struct ubus_client *cl, struct ubus_msg_bu
>>>        if (obj->type && obj->type->refcount == 1)
>>>                blob_put_int32(&b, UBUS_ATTR_OBJTYPE, obj->type->id.id);
>>> 
>>> -       ubusd_free_object(obj);
>>>        ubus_proto_send_msg_from_blob(cl, ub, UBUS_MSG_DATA);
>>> +       ubusd_free_object(obj);
>> I've added some fixes for NULL pointer dereference issues, however I
>> don't quite understand how this patch changes anything. As far as I can
>> tell, ubus_proto_send_msg_from_blob does not access obj.
>> Could you please provide some more context for your proposed fix?
> 
> I’ll try explaining again but knowing which bit of my original
> explanation isn’t clear would help me preparing a better answer… so here
> we go:
> 
> Both ubusd_free_object() (eventually via
> ubusd_create_object_event_msg()) and ubus_proto_send_msg_from_blob() use
> the same message buffer. So ubusd_handle_remove_object() builds the
> payload which gets (indirectly) overwritten by the call to
> ubusd_free_object() and then sent again by ubus_proto_send_msg_from_blob().
That was the part I was missing, now it makes sense to me. Fix pushed to
git, will update the ubus package soon.

Thanks,

- Felix