pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/41914?usp=email )
Change subject: osmo_io: Propagate segment_cb errors to the read_cb ......................................................................
osmo_io: Propagate segment_cb errors to the read_cb
Previous logic was to drop an entire msgb in the stream and pretend it can continue working that way, which clearly makes no sense. Instead, if segment_cb detects some problem (eg. buggy peer sending corrupted data according to protocol), propagate the issue through read_cb() so that the app can act on it, ie. most likely close the connection.
Related: SYS#7842 Change-Id: I572e68df6799b903507229a9beee6fa7d7d6d652 --- M src/core/osmo_io.c M tests/osmo_io/osmo_io_test.c M tests/osmo_io/osmo_io_test.err M tests/osmo_io/osmo_io_test.ok 4 files changed, 117 insertions(+), 11 deletions(-)
Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, approved
diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index 332a1e7..38814ae 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -297,8 +297,11 @@ }
/*! Handle segmentation of the msg. If this function returns *_HANDLE_ONE or MORE then the data in msg will contain - * one complete message. - * If there are bytes left over, *pending_out will point to a msgb with the remaining data. + * one complete message. + * If there are bytes left over, *pending_out will point to a msgb with the remaining data. + * Upon IOFD_SEG_ACT_DEFER is returned, errno is set to error value providing reason: + * EAGAIN is returned when data is still missing to fill the segment; other error codes are + * propagated through read_cb(). */ static enum iofd_seg_act iofd_handle_segmentation(struct osmo_io_fd *iofd, struct msgb *msg, struct msgb **pending_out) { @@ -319,15 +322,12 @@ return IOFD_SEG_ACT_HANDLE_ONE; }
- if (expected_len == -EAGAIN) { + if (expected_len < 0) { + if (expected_len != -EAGAIN) + LOGPIO(iofd, LOGL_ERROR, "segmentation_cb returned error (%d), skipping msg of size %d\n", + expected_len, received_len); + errno = -expected_len; goto defer; - } else if (expected_len < 0) { - /* Something is wrong, skip this msgb */ - LOGPIO(iofd, LOGL_ERROR, "segmentation_cb returned error (%d), skipping msg of size %d\n", - expected_len, received_len); - *pending_out = NULL; - msgb_free(msg); - return IOFD_SEG_ACT_DEFER; }
extra_len = received_len - expected_len; @@ -335,8 +335,11 @@ if (extra_len == 0) { *pending_out = NULL; return IOFD_SEG_ACT_HANDLE_ONE; + } + /* segment is incomplete */ - } else if (extra_len < 0) { + if (extra_len < 0) { + errno = EAGAIN; goto defer; }
@@ -456,6 +459,13 @@ return;
} else { /* IOFD_SEG_ACT_DEFER */ + if (OSMO_UNLIKELY(errno != EAGAIN)) { + /* Pass iofd->Pending to user app for debugging purposes: */ + msg = iofd->pending; + iofd->pending = NULL; + _call_read_cb(iofd, -errno, iofd->pending); + return; + } if (OSMO_UNLIKELY(msgb_length(iofd->pending) == iofd_msgb_length_max(iofd))) { LOGPIO(iofd, LOGL_ERROR, "Rx segment msgb of > %" PRIu16 " bytes (headroom %u bytes) is unsupported, check your segment_cb!\n", diff --git a/tests/osmo_io/osmo_io_test.c b/tests/osmo_io/osmo_io_test.c index f859da2..ca7fa67 100644 --- a/tests/osmo_io/osmo_io_test.c +++ b/tests/osmo_io/osmo_io_test.c @@ -558,6 +558,93 @@ osmo_select_main(1); }
+ +int _propagate_segmentation_cb(struct osmo_io_fd *iofd, struct msgb *msg) +{ + printf("%s: segmentation_cb() returning -ENOBUFS\n", osmo_iofd_get_name(iofd)); + return -ENOBUFS; +} + +static void propagate_read_cb(struct osmo_io_fd *iofd, int rc, struct msgb *msg) +{ + printf("%s: %s() msg with rc=%d msgb_len=%d error: %s\n", osmo_iofd_get_name(iofd), __func__, rc, + msg ? msgb_length(msg) : -1, strerror(-rc)); + OSMO_ASSERT(rc == -ENOBUFS); + if (msg) + talloc_free(msg); + + file_eof_read = true; + osmo_iofd_close(iofd); +} + +/* Test that errors other than EAGAIN are propagated to the user through the read_cb path. */ +static void test_segmentation_cb_propagate_error_to_read_cb(void) +{ + struct osmo_io_fd *iofd; + struct msgb *msg; + uint8_t *buf; + int fd[2] = { 0, 0 }; + int rc; + struct osmo_io_ops ioops; + + TEST_START(); + + /* Create pipe */ + rc = pipe(fd); + OSMO_ASSERT(rc == 0); + OSMO_ASSERT(fd[0]); + OSMO_ASSERT(fd[1]); + + /* First test writing to the pipe: */ + printf("Enable write\n"); + ioops = (struct osmo_io_ops){ .write_cb = file_write_cb }; + iofd = osmo_iofd_setup(ctx, fd[1], "seg_iofd", OSMO_IO_FD_MODE_READ_WRITE, &ioops, NULL); + osmo_iofd_register(iofd, fd[1]); + + msg = msgb_alloc(12, "Test data"); + buf = msgb_put(msg, 12); + memcpy(buf, TESTDATA, 12); + osmo_iofd_write_msgb(iofd, msg); + /* Allow enough cycles to handle the messages */ + file_bytes_write_compl = 0; + for (int i = 0; i < 128; i++) { + OSMO_ASSERT(file_bytes_write_compl <= 12); + if (file_bytes_write_compl == 12) + break; + osmo_select_main(1); + usleep(100 * 1000); + } + fflush(stdout); + OSMO_ASSERT(file_bytes_write_compl == 12); + + osmo_iofd_close(iofd); + + /* Now, re-configure iofd to only read from the pipe. + * Reduce the read buffer size, to verify correct segmentation operation: */ + printf("Enable read\n"); + osmo_iofd_set_alloc_info(iofd, 6, 0); + osmo_iofd_register(iofd, fd[0]); + ioops = (struct osmo_io_ops){ .read_cb = propagate_read_cb, .segmentation_cb2 = _propagate_segmentation_cb }; + rc = osmo_iofd_set_ioops(iofd, &ioops); + OSMO_ASSERT(rc == 0); + /* Allow enough cycles to handle the message. We expect 3 reads, 4th read will return 0. */ + file_bytes_read = 0; + file_eof_read = false; + for (int i = 0; i < 128; i++) { + if (file_eof_read) + break; + osmo_select_main(1); + usleep(100 * 1000); + } + fflush(stdout); + OSMO_ASSERT(file_eof_read); + + osmo_iofd_free(iofd); + + for (int i = 0; i < 128; i++) + osmo_select_main(1); +} + static const struct log_info_cat default_categories[] = { };
@@ -587,6 +674,7 @@ test_segmentation_uint16_max(10000, 3000, 0); test_segmentation_cb_requests_segment_too_big(0); test_segmentation_cb_requests_segment_too_big(320); + test_segmentation_cb_propagate_error_to_read_cb();
return EXIT_SUCCESS; } diff --git a/tests/osmo_io/osmo_io_test.err b/tests/osmo_io/osmo_io_test.err index dc7cde9..9898c93 100644 --- a/tests/osmo_io/osmo_io_test.err +++ b/tests/osmo_io/osmo_io_test.err @@ -1,2 +1,3 @@ ERROR iofd(seg_iofd) Rx segment msgb of > 65535 bytes (headroom 0 bytes) is unsupported, check your segment_cb! ERROR iofd(seg_iofd) Rx segment msgb of > 65215 bytes (headroom 320 bytes) is unsupported, check your segment_cb! +ERROR iofd(seg_iofd) segmentation_cb returned error (-105), skipping msg of size 6 diff --git a/tests/osmo_io/osmo_io_test.ok b/tests/osmo_io/osmo_io_test.ok index 3c2873f..d368db4 100644 --- a/tests/osmo_io/osmo_io_test.ok +++ b/tests/osmo_io/osmo_io_test.ok @@ -334,3 +334,10 @@ seg_iofd: segmentation_cb_requests_segment_too_big_segmentation_cb() returning 65216 seg_iofd: segmentation_cb_requests_segment_too_big_segmentation_cb() returning 65216 seg_iofd: segmentation_cb_requests_segment_too_big_read_cb() msg with rc=-71 msgb_len=65215 error: Protocol error +Running test_segmentation_cb_propagate_error_to_read_cb +Enable write +seg_iofd: write() returned rc=12 +01 02 03 04 05 06 07 08 09 0a 0b 0c +Enable read +seg_iofd: segmentation_cb() returning -ENOBUFS +seg_iofd: propagate_read_cb() msg with rc=-105 msgb_len=-1 error: No buffer space available