Use the correct parameters for list_add_tail when enqueueing a message
in the tx_queue. Previously, list_add_tail(list, entry) was used instead
of list_add_tail(entry, list). Due to this, the list would only contain
the latest entry, and previously inserted entries were left dangling.
Reset the tx_queue offset after a message from the tx_queue has been sent
completely.
Signed-off-by: Alexander Van Parys <alexander.vanparys_ext@softathome.com>
No new message can be enqueued if this brings the total queue length of
that client over UBUS_CLIENT_MAX_TXQ_LEN.
Set UBUS_CLIENT_MAX_TXQ_LEN to UBUS_MAX_MSGLEN, i.e. 1MB. This limit
should be plenty for any practical use case.
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
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>
In commit 5d7ca8309d ("ubusd/libubus-io: fix variable sized struct
position warning") the position of cmsghdr struct has been changed in
order to fix clang-9 compiler warning, but it has introduced regression
in at least `logread` which hanged indefinitely.
So this patch reworks the socket descriptor passing in a way recommended
in the `cmsg(3)` manual page.
Ref: http://lists.infradead.org/pipermail/openwrt-devel/2019-December/020840.html
Fixes: 5d7ca8309d ("ubusd/libubus-io: fix variable sized struct position warning")
Reported-by: Hannu Nyman <hannu.nyman@welho.com>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
clang-9 on x86/64 has reported following warnings/errors:
libubus-acl.c:123:2: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
libubus-io.c:108:18: error: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
libubus-io.c:395:56: error: comparison of integers of different signs: 'ssize_t' (aka 'long') and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
libubus-req.c:441:4: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
ubusd_acl.c:119:18: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
ubusd_acl.c:152:5: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
ubusd_acl.c:348:3: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
ubusd_acl.c:352:3: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
ubusd_acl.c:357:3: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
ubusd_acl.c:362:3: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
ubusd_acl.c:367:3: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
ubusd_acl.c:447:16: error: comparison of integers of different signs: 'int' and '__size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
ubusd_acl.c:502:18: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
ubusd.c:123:13: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
ubusd.c:170:15: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
ubusd.c:262:43: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
ubusd.c:287:30: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
ubusd_event.c:170:18: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
ubusd_obj.c:71:2: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fixes following clang-9 compiler warnings:
ubusd.c:99:18: error: field 'h' with variable sized type 'struct cmsghdr' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
struct cmsghdr h;
^
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fixes following clang-9 compiler warning:
ubusd.c:36:19: error: comparison of integers of different signs: 'uint32_t' (aka 'unsigned int') and 'int' [-Werror,-Wsign-compare]
if (ub->refcount == ~0) {
~~~~~~~~~~~~ ^ ~~
Signed-off-by: Petr Štetiar <ynezz@true.cz>
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>
For a shared ubus_msg_buf, the ubus_msg_ref function will
create a copy for queue-ing.
Problem is, that during the dequeue (especially) in client_cb,
the header is 0-ed (because it's was a newly alloc-ed buffer).
And during ubus_msg_writev(), the header info will be ignored
by the client.
Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
An -1 returned by ubus_msg_writev() will be interpreted as
UINT_MAX during a check to see how much data had could be
written on the socket.
Because sizeof() will return size_t it will promote the
comparsion to unsigned
Signed-off-by: Mihai Richard <mihairichard@live.com>
62cdfc3 added systemd units including a ubus.socket unit, but didn't
actually add socket activation support to ubusd. This would cause the first
connection that activated ubusd to hang (as ubusd ignored it), and stopping
ubusd would break it completely (as ubusd removed the socket file).
The ENABLE_SYSTEMD default is changed to OFF as the socket activation uses
libsystemd, so setting ENABLE_SYSTEMD to ON will now require libsystemd.
Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
Changing the ubus message header fields from 'host' order to 'network' order
allows passing ubus messages between hosts with different endianity.
Example use (creating a ubus proxy):
on host A (e.g. big endian router already running ubusd), run:
$ socat TCP-LISTEN:5699,fork UNIX:/var/run/ubus.sock &
On host B (e.g. little endian development PC) run:
$ socat UNIX-LISTEN:/var/run/ubus.sock,fork TCP:<host A IP>:5699 &
Now ubus applications can be run on host B and seamlessly interact with ubus
applications on host A.
Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
The realloc is problematic mostly with large packets, as the pointer changes
so what eventually gets free'd is invalid.
Note that ub ptr param in the call will be passed on to a ubus_msg_free(),
right after ubus_msg_ref() finishes.
This bug stayed hidden the same way as the bug in libubus writev_retry().
Since the write/sendmsg function can send about ~200k the ubus_msg_enqueue()
call does not get triggered.
