On 2016-08-23 12:38, Delio Brignoli wrote: > Hi Felix, > >> On 23 Aug 2016, at 11:40, Felix Fietkau <n...@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 _______________________________________________ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
