ubusd maintains a per-client tx_queue containing references to message
buffers that have not been sent yet (due to the socket blocking). This
is a fixed-size, 64-element queue.
When more than 64 elements are queued, subsequent elements are simply
dropped. Thus, a client that is waiting for those messages will block
indefinitely. In particular, this happens when more than +- 250 objects
are registered on the bus and either "ubus list" or "ubus wait_for" is
called. The responses to these requests consist of a message buffer per
object. Since in practice, ubusd will not yield between the sends of
these message buffers, the client has no time to process them and
eventually the output socket blocks. After 64 more objects, the rest is
dropped, including the final message that indicates termination. Thus,
the client waits indefinitely for the termination message.
To solve this, turn the tx_queue into a variable-sized linked list
instead of a fixed-size queue.
To maintain the linked list, an additional structure ubus_msg_buf_list
is created. It is not possible to add the linked list to ubus_msg_buf,
because that is shared between clients.
Note that this infinite tx_queue opens the door to a DoS attack. You can
open a client and a server connection, then send messages from the
client to the server without ever reading anything on the server side.
This will eventually lead to an out-of-memory. However, such a DoS
already existed anyway, it just requires opening multiple server
connections and filling up the fixed-size queue on each one. To protect
against such DoS attacks, we'd need to:
- keep a global maximum queue size that applies to all rx and tx queues
together;
- stop reading from any connection when the maximum is reached;
- close any connection when it hasn't become writeable after some
timeout.
Fixes: https://bugs.openwrt.org/index.php?do=details&task_id=1525
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
blob_parse expects blobs from trusted inputs, but it can be supplied
with possibly malicious blobs from untrusted inputs as well, which might
lead to undefined behaviour and/or crash of ubus daemon. In order to
prevent such conditions, switch to blob_parse_untrusted which should
hopefully handle such untrusted inputs appropriately.
Signed-off-by: Petr Štetiar <ynezz@true.cz>
scan-build from clang-9 has reported following:
libubox/list.h:83:22: warning: Use of memory after it is freed
entry->next->prev = entry->prev;
^~~~~~~~~~~
ubusd_event.c:42:3: warning: Use of memory after it is freed
ubusd_delete_event_source(ev);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Which might be a false positives, but in order to make the code pass the
static analyzer checks, rewrite the while loops on lists with the safe
list iterator.
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Even with the tx_queue-ing issue resolved, what
seems to happen afterwards, is that all the messages
seems to get through, but the client still loops
in the `ubus_complete_request()` waiting for
`req->status_msg` or for a timeout.
Though, the timeout does not seem to happen, because
the data is processed in `ubus_poll_data()`, with
a infinite poll() timeout (ubus_complete_request() is
called with timeout 0).
It's likely that either the `seq` or `peer` sent from
ubusd are wrong, and the client cannot get the correct
ubus request in `ubus_process_req_msg()`.
I haven't digged too deep into this ; setting the
`retmsg` object on the client struct seems to have
resolved any hanging with the `ubus list` command.
Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: Felix Fietkau <nbd@nbd.name> [fix placement of retmsg in cl]
This makes it clear that `ubus_msg_send()` is only
about sending and queue-ing messages, and has nothing
to do with free-ing.
It can be a bit misleading/confusing when trying to go
through the code and make assumptions about whether a
buffer is free'd in ubus_send_msg(), or is free'd outside.
In `ubusd_proto_receive_message()` the `ubus_msg_free()`
is now called before the `if (ret == -1)` check.
That way, all callbacks will have their messages free'd,
which is what's desired, but confusing, because:
* ubusd_handle_invoke() called ubus_msg_free() before returning -1
* ubusd_handle_notify() called ubus_msg_free() before returning -1
* ubusd_handle_response() called ubus_msg_send(,,free=true) before returning -1
* ubus_msg_send() would call ubus_msg_send(,,free=false)
* all other callback callers would `ubus_msg_send(,,free=true)`
(free the buffers in ubus_msg_send() )
In all other places, where `ubus_msg_send(,,free=true)`
an explicit `ubus_msg_free()` was added.
Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Patch by Delio Brignoli <brignoli.delio@gmail.com>
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.
Signed-off-by: Felix Fietkau <nbd@nbd.name>
__init has a naming collision with C++ and prevents ubus_common.h
from being included. Instead, use __constructor as defined from
libubox.
Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
rename the ADD_WATCH/REMOVE_WATCH messages to SUBSCRIBE/UNSUBSCRIBE and change
the message format and libubus API in preparation for adding object notifications
Signed-off-by: Felix Fietkau <nbd@openwrt.org>
