[M] Change in libosmocore[master]: soft_uart: fix the Rx flushing logic, add a unit test

fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/35297?usp=email ) Change subject: soft_uart: fix the Rx flushing logic, add a unit test ...................................................................... soft_uart: fix the Rx flushing logic, add a unit test Coverity tells us that with the current logic it's possible (in theory) that we may dereference NULL pointer in osmo_soft_uart_flush_rx(). This is highly unlikely, because the Rx buffer gets allocated once when the Rx is enabled and remains even after the Rx gets disabled. The Rx flags cannot be anything than 0x00 before the Rx gets enabled. Even though this NULL pointer dereference is unlikely, the Rx flushing logic is still not entirely correct. As can be seen from the unit test output, the Rx callback of the application may be called with an empty msgb if the following conditions are both met: a) the osmo_soft_uart_flush_rx() is invoked manually, and b) a parity and/or a framing error has occurred previously. We should not be checking suart->rx.flags in osmo_soft_uart_flush_rx(), since this is already done in suart_rx_ch(), which is calling it. Removing this check also eliminates a theoretical possibility of the NULL pointer dereference, so we're killing two birds with one stone. - Do not check suart->rx.flags in osmo_soft_uart_flush_rx(). - Add a unit test for various flush()ing scenarios. Change-Id: I5179f5fd2361e4e96ac9bf48e80b99e53a7e4712 Fixes: CID#336545 --- M src/core/soft_uart.c M tests/soft_uart/soft_uart_test.c M tests/soft_uart/soft_uart_test.ok 3 files changed, 60 insertions(+), 3 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/97/35297/1 diff --git a/src/core/soft_uart.c b/src/core/soft_uart.c index c6a6dbd..f969ab7 100644 --- a/src/core/soft_uart.c +++ b/src/core/soft_uart.c @@ -79,7 +79,7 @@ * \param[in] suart soft-UART instance holding the receive buffer. */ void osmo_soft_uart_flush_rx(struct osmo_soft_uart *suart) { - if ((suart->rx.msg && msgb_length(suart->rx.msg)) || suart->rx.flags) { + if (suart->rx.msg && msgb_length(suart->rx.msg)) { osmo_timer_del(&suart->rx.timer); if (suart->cfg.rx_cb) { suart->cfg.rx_cb(suart->cfg.priv, suart->rx.msg, suart->rx.flags); diff --git a/tests/soft_uart/soft_uart_test.c b/tests/soft_uart/soft_uart_test.c index c8aef15..43f49b9 100644 --- a/tests/soft_uart/soft_uart_test.c +++ b/tests/soft_uart/soft_uart_test.c @@ -224,6 +224,27 @@ osmo_soft_uart_free(suart); } +static void test_rx_flush(void) +{ + struct osmo_soft_uart *suart; + + SUART_TEST_BEGIN; + + suart = osmo_soft_uart_alloc(NULL, __func__, &suart_test_default_cfg); + OSMO_ASSERT(suart != NULL); + + printf("calling osmo_soft_uart_flush_rx() while Rx disabled\n"); + osmo_soft_uart_flush_rx(suart); + + printf("enabling the receiver\n"); + osmo_soft_uart_set_rx(suart, true); + + printf("calling osmo_soft_uart_flush_rx() while Rx enabled, but no data\n"); + osmo_soft_uart_flush_rx(suart); + + osmo_soft_uart_free(suart); +} + static void test_tx_rx_exec_one(struct osmo_soft_uart *suart, size_t n_bits_total, size_t n_bits_frame) { @@ -566,6 +587,7 @@ int main(int argc, char **argv) { test_rx(); + test_rx_flush(); test_tx_rx(); /* test pulling small number of bits at a time */ diff --git a/tests/soft_uart/soft_uart_test.ok b/tests/soft_uart/soft_uart_test.ok index 6edac16..c42d0f5 100644 --- a/tests/soft_uart/soft_uart_test.ok +++ b/tests/soft_uart/soft_uart_test.ok @@ -51,7 +51,6 @@ suart_rx_cb(flags=02): 01 suart_rx_cb(flags=02): ff test_rx_exec() @ 49: flush the Rx buffer -suart_rx_cb(flags=02): ======== testing 8-E-1 (valid parity) test_rx_exec() @ 63: flush the Rx buffer suart_rx_cb(flags=00): 00 ff aa 55 @@ -62,13 +61,17 @@ suart_rx_cb(flags=02): 01 suart_rx_cb(flags=02): ff test_rx_exec() @ 42: flush the Rx buffer -suart_rx_cb(flags=02): ======== testing 8-O-1 (valid parity) test_rx_exec() @ 63: flush the Rx buffer suart_rx_cb(flags=00): 00 ff aa 55 test_rx_exec() @ 120: flush the Rx buffer suart_rx_cb(flags=00): 80 e0 f8 fe +Executing test_rx_flush +calling osmo_soft_uart_flush_rx() while Rx disabled +enabling the receiver +calling osmo_soft_uart_flush_rx() while Rx enabled, but no data + Executing test_tx_rx ======== testing 8-N-1 suart_tx_cb(len=4/4): de ad be ef -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35297?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I5179f5fd2361e4e96ac9bf48e80b99e53a7e4712 Gerrit-Change-Number: 35297 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: newchange