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 &lt;pespin(a)sysmocom.de&gt;