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
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/41914?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I572e68df6799b903507229a9beee6fa7d7d6d652
Gerrit-Change-Number: 41914
Gerrit-PatchSet: 8
Gerrit-Owner: pespin <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <[email protected]>
Gerrit-Reviewer: fixeria <[email protected]>
Gerrit-Reviewer: laforge <[email protected]>
Gerrit-Reviewer: pespin <[email protected]>
