[L] Change in libosmo-netif[master]: stream_cli: Support destroy object within user callback

22 Nov 2024

pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-netif/+/38911?usp=email )
Change subject: stream_cli: Support destroy object within user callback
......................................................................
stream_cli: Support destroy object within user callback
read_cb(len=0) -> osmo_stream_cli_reconnect() -> osmo_stream_cli_close() -> disconnect_cb -> [user calls osmo_stream_cli_destroy()] -> [popped stack does read_cb()] -> [user uses freed msg]
Change-Id: I952938474fa2780bf3c906cbdffb2d024b03c1b7
---
M src/stream_cli.c
1 file changed, 174 insertions(+), 88 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/11/38911/1

diff --git a/src/stream_cli.c b/src/stream_cli.c
index e70bc35..79d0e8d 100644
--- a/src/stream_cli.c
+++ b/src/stream_cli.c
@@ -83,6 +83,11 @@
 #define OSMO_STREAM_CLI_F_RECONF	(1 << 0)
 #define OSMO_STREAM_CLI_F_NODELAY	(1 << 1)
+/* Mark whether the object is currently in user a callback. */
+#define IN_CB_MASK_CONNECT_CB		(1 << 0)
+#define IN_CB_MASK_DISCONNECT_CB	(1 << 1)
+#define IN_CB_MASK_READ_CB		(1 << 2)
+
 struct osmo_stream_cli {
    char *name;
    char sockname[OSMO_SOCK_NAME_MAXLEN];
@@ -114,31 +119,128 @@
    int				flags;
    int				reconnect_timeout;
    struct osmo_sock_init2_multiaddr_pars ma_pars;
+	uint8_t				in_cb_mask; /* IN_CB_MASK_* */
+	bool				delay_free;
 };
-void osmo_stream_cli_close(struct osmo_stream_cli *cli);
/*! \addtogroup stream_cli
  *  @{
  */
+/* return true if freed */
+static inline bool free_delayed_if_needed(struct osmo_stream_cli *cli)
+{
+	/* Nobody requested delayed free, skip */
+	if (!cli->delay_free)
+		return false;
+	/* Check for other callbacks active in case we were e.g. in:
+	* read_cb() -> [user] -> osmo_steam_client_close() -> disconnect_cb() --> [user] --> osmo_stream_client_destroy()
+	* or:
+	* connect_cb() -> [user] -> osmo_steam_client_close() -> disconnect_cb() --> [user] --> osmo_stream_client_destroy()
+	*/
+	if (cli->in_cb_mask != 0)
+		return false;
+
+	LOGSCLI(cli, LOGL_DEBUG, "free(delayed)\n");
+	talloc_free(cli);
+	return true;
+}
+
+static void stream_cli_close_iofd(struct osmo_stream_cli *cli)
+{
+	if (!cli->iofd)
+		return;
+
+	osmo_iofd_free(cli->iofd);
+	cli->iofd = NULL;
+}
+
+static void stream_cli_close_ofd(struct osmo_stream_cli *cli)
+{
+	if (cli->ofd.fd == -1)
+		return;
+	osmo_fd_unregister(&cli->ofd);
+	close(cli->ofd.fd);
+	cli->ofd.fd = -1;
+}
+
+/*! Close an Osmocom Stream Client.
+ *  \param[in] cli Osmocom Stream Client to be closed
+ *  \return true if stream was freed due to disconnect_cb, false otherwise
+ *  We unregister the socket fd from the osmocom select() loop
+ *  abstraction and close the socket */
+static bool stream_cli_close(struct osmo_stream_cli *cli)
+{
+	int old_state = cli->state;
+
+	/* This guards against reentrant close through disconnect_cb(): */
+	if (cli->state == STREAM_CLI_STATE_CLOSED)
+		return false;
+	if (cli->state == STREAM_CLI_STATE_WAIT_RECONNECT) {
+		osmo_timer_del(&cli->timer);
+		cli->state = STREAM_CLI_STATE_CLOSED;
+		return false;
+	}
+
+
+	switch (cli->mode) {
+	case OSMO_STREAM_MODE_OSMO_FD:
+		stream_cli_close_ofd(cli);
+		break;
+	case OSMO_STREAM_MODE_OSMO_IO:
+		stream_cli_close_iofd(cli);
+		break;
+	default:
+		OSMO_ASSERT(false);
+	}
+
+	cli->state = STREAM_CLI_STATE_CLOSED;
+
+	/* If conn was established, notify the disconnection to the user:
+	 * Also, if reconnect is disabled by user, notify the user that connect() failed: */
+	if (old_state == STREAM_CLI_STATE_CONNECTED ||
+	    (old_state == STREAM_CLI_STATE_CONNECTING && cli->reconnect_timeout < 0)) {
+		LOGSCLI(cli, LOGL_DEBUG, "connection closed\n");
+		cli->in_cb_mask |= IN_CB_MASK_DISCONNECT_CB;
+		if (cli->disconnect_cb)
+			cli->disconnect_cb(cli);
+		cli->in_cb_mask &= ~IN_CB_MASK_DISCONNECT_CB;
+		return free_delayed_if_needed(cli);
+	}
+	return false;
+}
+
+/*! Re-connect an Osmocom Stream Client.
+ *  If re-connection is enabled for this client
+ *  (which is the case unless negative timeout was explicitly set via osmo_stream_cli_set_reconnect_timeout() call),
+ *  we close any existing connection (if any) and schedule a re-connect timer */
+static bool stream_cli_reconnect(struct osmo_stream_cli *cli)
+{
+	bool freed = stream_cli_close(cli);
+
+	if (freed)
+		return true;
+
+	if (cli->reconnect_timeout < 0) {
+		LOGSCLI(cli, LOGL_INFO, "not reconnecting, disabled\n");
+		return false;
+	}
+
+	cli->state = STREAM_CLI_STATE_WAIT_RECONNECT;
+	LOGSCLI(cli, LOGL_INFO, "retrying reconnect in %d seconds...\n",
+		cli->reconnect_timeout);
+	osmo_timer_schedule(&cli->timer, cli->reconnect_timeout, 0);
+	return false;
+}
+
 /*! Re-connect an Osmocom Stream Client.
  *  If re-connection is enabled for this client
  *  (which is the case unless negative timeout was explicitly set via osmo_stream_cli_set_reconnect_timeout() call),
  *  we close any existing connection (if any) and schedule a re-connect timer */
 void osmo_stream_cli_reconnect(struct osmo_stream_cli *cli)
 {
-	osmo_stream_cli_close(cli);
-
-	if (cli->reconnect_timeout < 0) {
-		LOGSCLI(cli, LOGL_INFO, "not reconnecting, disabled\n");
-		return;
-	}
-
-	cli->state = STREAM_CLI_STATE_WAIT_RECONNECT;
-	LOGSCLI(cli, LOGL_INFO, "retrying reconnect in %d seconds...\n",
-		cli->reconnect_timeout);
-	osmo_timer_schedule(&cli->timer, cli->reconnect_timeout, 0);
+	stream_cli_reconnect(cli);
 }
/*! Check if Osmocom Stream Client is in connected state.
@@ -161,62 +263,13 @@
           cli->state == STREAM_CLI_STATE_CONNECTED;
 }
-static void osmo_stream_cli_close_iofd(struct osmo_stream_cli *cli)
-{
-	if (!cli->iofd)
-		return;
-
-	osmo_iofd_free(cli->iofd);
-	cli->iofd = NULL;
-}
-
-static void osmo_stream_cli_close_ofd(struct osmo_stream_cli *cli)
-{
-	if (cli->ofd.fd == -1)
-		return;
-	osmo_fd_unregister(&cli->ofd);
-	close(cli->ofd.fd);
-	cli->ofd.fd = -1;
-}
-
 /*! Close an Osmocom Stream Client.
  *  \param[in] cli Osmocom Stream Client to be closed
  *  We unregister the socket fd from the osmocom select() loop
  *  abstraction and close the socket */
 void osmo_stream_cli_close(struct osmo_stream_cli *cli)
 {
-	int old_state = cli->state;
-
-	if (cli->state == STREAM_CLI_STATE_CLOSED)
-		return;
-	if (cli->state == STREAM_CLI_STATE_WAIT_RECONNECT) {
-		osmo_timer_del(&cli->timer);
-		cli->state = STREAM_CLI_STATE_CLOSED;
-		return;
-	}
-
-
-	switch (cli->mode) {
-	case OSMO_STREAM_MODE_OSMO_FD:
-		osmo_stream_cli_close_ofd(cli);
-		break;
-	case OSMO_STREAM_MODE_OSMO_IO:
-		osmo_stream_cli_close_iofd(cli);
-		break;
-	default:
-		OSMO_ASSERT(false);
-	}
-
-	cli->state = STREAM_CLI_STATE_CLOSED;
-
-	/* If conn was established, notify the disconnection to the user:
-	 * Also, if reconnect is disabled by user, notify the user that connect() failed: */
-	if (old_state == STREAM_CLI_STATE_CONNECTED ||
-	    (old_state == STREAM_CLI_STATE_CONNECTING && cli->reconnect_timeout < 0)) {
-		LOGSCLI(cli, LOGL_DEBUG, "connection closed\n");
-		if (cli->disconnect_cb)
-			cli->disconnect_cb(cli);
-	}
+	stream_cli_close(cli);
 }
/*! Retrieve file descriptor of the stream client socket.
@@ -249,15 +302,19 @@
    return cli->iofd;
 }
-static void osmo_stream_cli_read(struct osmo_stream_cli *cli)
+/* Return true if read_cb caused a delayed_free, hence cli not available anymore. */
+static bool stream_cli_read(struct osmo_stream_cli *cli)
 {
    LOGSCLI(cli, LOGL_DEBUG, "message received\n");
+	cli->in_cb_mask |= IN_CB_MASK_READ_CB;
    if (cli->read_cb)
    	cli->read_cb(cli);
+	cli->in_cb_mask &= ~IN_CB_MASK_READ_CB;
+	return free_delayed_if_needed(cli);
 }
-static int osmo_stream_cli_write(struct osmo_stream_cli *cli)
+static int stream_cli_write(struct osmo_stream_cli *cli)
 {
 #ifdef HAVE_LIBSCTP
    struct sctp_sndrcvinfo sinfo;
@@ -323,7 +380,7 @@
    		return 0;
    	}
    	msgb_free(msg);
-		osmo_stream_cli_reconnect(cli);
+		stream_cli_reconnect(cli);
    	return 0;
    }
@@ -359,13 +416,13 @@
if (ret < 0) {
    	LOGSCLI(cli, LOGL_ERROR, "connect failed (%d)\n", res);
-		osmo_stream_cli_reconnect(cli);
+		stream_cli_reconnect(cli);
    	return;
    }
    ret = getsockopt(fd, SOL_SOCKET, SO_ERROR, &error, &len);
    if (ret >= 0 && error > 0) {
    	LOGSCLI(cli, LOGL_ERROR, "connect so_error (%d)\n", error);
-		osmo_stream_cli_reconnect(cli);
+		stream_cli_reconnect(cli);
    	return;
    }
@@ -394,8 +451,11 @@
    default:
    	break;
    }
+	cli->in_cb_mask |= IN_CB_MASK_CONNECT_CB;
    if (cli->connect_cb)
    	cli->connect_cb(cli);
+	cli->in_cb_mask &= ~IN_CB_MASK_CONNECT_CB;
+	free_delayed_if_needed(cli);
 }
static int osmo_stream_cli_fd_cb(struct osmo_fd *ofd, unsigned int what)
@@ -409,11 +469,12 @@
    case STREAM_CLI_STATE_CONNECTED:
    	if (what & OSMO_FD_READ) {
    		LOGSCLI(cli, LOGL_DEBUG, "connected read\n");
-			osmo_stream_cli_read(cli);
+			if (stream_cli_read(cli) == true)
+				break; /* cli (and hence ofd) freed, done. */
    	}
    	if (what & OSMO_FD_WRITE) {
    		LOGSCLI(cli, LOGL_DEBUG, "connected write\n");
-			osmo_stream_cli_write(cli);
+			stream_cli_write(cli);
    	}
    	break;
    default:
@@ -459,6 +520,7 @@
 static void stream_cli_iofd_read_cb(struct osmo_io_fd *iofd, int res, struct msgb *msg)
 {
    struct osmo_stream_cli *cli  = osmo_iofd_get_data(iofd);
+	bool freed;
switch (cli->state) {
    case STREAM_CLI_STATE_CONNECTING:
@@ -470,21 +532,29 @@
    	case -EPIPE:
    	case -ECONNRESET:
    		LOGSCLI(cli, LOGL_ERROR, "lost connection with srv (%d)\n", res);
-			osmo_stream_cli_reconnect(cli);
+			freed = stream_cli_reconnect(cli);
    		break;
    	case 0:
    		LOGSCLI(cli, LOGL_NOTICE, "connection closed with srv\n");
-			osmo_stream_cli_reconnect(cli);
+			freed = stream_cli_reconnect(cli);
    		break;
    	default:
    		LOGSCLI(cli, LOGL_DEBUG, "received %d bytes from srv\n", res);
+			freed = false;
    		break;
    	}
+		if (freed)
+			return; /* msg was also freed as part of the talloc tree. */
    	/* Notify user of new data or error: */
-		if (cli->iofd_read_cb)
-			cli->iofd_read_cb(cli, res, msg);
-		else
+		if (!cli->iofd_read_cb) {
    		msgb_free(msg);
+			return;
+		}
+		cli->in_cb_mask |= IN_CB_MASK_READ_CB;
+		cli->iofd_read_cb(cli, res, msg);
+		cli->in_cb_mask &= ~IN_CB_MASK_READ_CB;
+		OSMO_ASSERT(cli->in_cb_mask == 0);
+		free_delayed_if_needed(cli);
    	break;
    default:
    	osmo_panic("%s() called with unexpected state %d\n", __func__, cli->state);
@@ -501,8 +571,8 @@
    	break;
    case STREAM_CLI_STATE_CONNECTED:
    	if (msg && res <= 0) {
-			osmo_stream_cli_reconnect(cli);
    		LOGSCLI(cli, LOGL_ERROR, "received error %d in response to send\n", res);
+			stream_cli_reconnect(cli);
    	}
    	break;
    default:
@@ -521,6 +591,7 @@
 static void stream_cli_iofd_recvmsg_cb(struct osmo_io_fd *iofd, int res, struct msgb *msg, const struct msghdr *msgh)
 {
    struct osmo_stream_cli *cli  = osmo_iofd_get_data(iofd);
+	bool freed;
res = stream_iofd_sctp_recvmsg_trailer(iofd, msg, res, msgh);
@@ -534,20 +605,28 @@
    	case -EPIPE:
    	case -ECONNRESET:
    		LOGSCLI(cli, LOGL_ERROR, "lost connection with srv (%d)\n", res);
-			osmo_stream_cli_reconnect(cli);
+			freed = stream_cli_reconnect(cli);
    		break;
    	case 0:
    		LOGSCLI(cli, LOGL_NOTICE, "connection closed with srv\n");
-			osmo_stream_cli_reconnect(cli);
+			freed = stream_cli_reconnect(cli);
    		break;
    	default:
+			freed = 0;
    		break;
    	}
+		if (freed)
+			return; /* msg was also freed as part of the talloc tree. */
    	/* Notify user of new data or error: */
-		if (cli->iofd_read_cb)
-			cli->iofd_read_cb(cli, res, msg);
-		else
+		if (!cli->iofd_read_cb) {
    		msgb_free(msg);
+			return;
+		}
+		cli->in_cb_mask |= IN_CB_MASK_READ_CB;
+		cli->iofd_read_cb(cli, res, msg);
+		cli->in_cb_mask &= ~IN_CB_MASK_READ_CB;
+		OSMO_ASSERT(cli->in_cb_mask == 0);
+		free_delayed_if_needed(cli);
    	break;
    default:
    	osmo_panic("%s() called with unexpected state %d\n", __func__, cli->state);
@@ -863,10 +942,17 @@
 void osmo_stream_cli_destroy(struct osmo_stream_cli *cli)
 {
    LOGSCLI(cli, LOGL_DEBUG, "destroy()\n");
-	osmo_stream_cli_close(cli);
+	OSMO_ASSERT(!stream_cli_close(cli)); /* BUG IN APP, TRYING TO DESTROY() WHILE ALREADY IN DESTROY()*/
    osmo_timer_del(&cli->timer);
    msgb_queue_free(&cli->tx_queue);
-	talloc_free(cli);
+	/* if we are in a user callback, delay freeing. */
+	if (cli->in_cb_mask != 0) {
+		LOGSCLI(cli, LOGL_DEBUG, "delay free() in_cb_mask=0x%02x\n", cli->in_cb_mask);
+		cli->delay_free = true;
+	} else {
+		LOGSCLI(cli, LOGL_DEBUG, "free(destroy)\n");
+		talloc_free(cli);
+	}
 }
/*! DEPRECATED: use osmo_stream_cli_set_reconnect_timeout() or osmo_stream_cli_reconnect() instead!
@@ -881,7 +967,7 @@
/* we are reconfiguring this socket, close existing first. */
    if ((cli->flags & OSMO_STREAM_CLI_F_RECONF) && cli->ofd.fd >= 0)
-		osmo_stream_cli_close(cli);
+		stream_cli_close(cli);
cli->flags &= ~OSMO_STREAM_CLI_F_RECONF;
@@ -907,7 +993,7 @@
    if (ret < 0) {
    	LOGSCLI(cli, LOGL_ERROR, "connect: socket creation error (%d)\n", ret);
    	if (reconnect)
-			osmo_stream_cli_reconnect(cli);
+			stream_cli_reconnect(cli);
    	return ret;
    }
    osmo_fd_setup(&cli->ofd, ret, OSMO_FD_READ | OSMO_FD_WRITE, osmo_stream_cli_fd_cb, cli, 0);
@@ -1035,7 +1121,7 @@
/* we are reconfiguring this socket, close existing first. */
    if ((cli->flags & OSMO_STREAM_CLI_F_RECONF) && osmo_stream_cli_get_fd(cli) >= 0)
-		osmo_stream_cli_close(cli);
+		stream_cli_close(cli);
cli->flags &= ~OSMO_STREAM_CLI_F_RECONF;
@@ -1078,7 +1164,7 @@
if (ret < 0) {
    	LOGSCLI(cli, LOGL_ERROR, "connect: socket creation error (%d)\n", ret);
-		osmo_stream_cli_reconnect(cli);
+		stream_cli_reconnect(cli);
    	return ret;
    }
@@ -1098,7 +1184,7 @@
    	break;
    case OSMO_STREAM_MODE_OSMO_IO:
    	/* Be sure that previous osmo_io instance is freed before creating a new one. */
-		osmo_stream_cli_close_iofd(cli);
+		stream_cli_close_iofd(cli);
 #ifdef HAVE_LIBSCTP
    	if (cli->proto == IPPROTO_SCTP) {
    		cli->iofd = osmo_iofd_setup(cli, fd, cli->name, OSMO_IO_FD_MODE_RECVMSG_SENDMSG,
@@ -1242,7 +1328,7 @@
    		LOGSCLI(cli, LOGL_ERROR, "lost connection with srv (%d)\n", errno);
    	else
    		LOGSCLI(cli, LOGL_ERROR, "recv failed (%d)\n", errno);
-		osmo_stream_cli_reconnect(cli);
+		stream_cli_reconnect(cli);
    	return ret;
    } else if (ret == 0) {
    	LOGSCLI(cli, LOGL_ERROR, "connection closed with srv\n");
-- 
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/38911?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I952938474fa2780bf3c906cbdffb2d024b03c1b7
Gerrit-Change-Number: 38911
Gerrit-PatchSet: 1
Gerrit-Owner: pespin pespin@sysmocom.de



    

2025

2024

2023

2022

[L] Change in libosmo-netif[master]: stream_cli: Support destroy object within user callback