[v2,1/1] write_queue: Avoid possible use-after-free if fd is read-/writable
diff mbox

Message ID aa989a94b42e06a7e2bbf1761a7049b9e3db508e.1402387344.git.daniel@totalueberwachung.de
State Accepted
Headers show

Commit Message

Daniel Willmann June 10, 2014, 8:02 a.m. UTC
From: Daniel Willmann <dwillmann@sysmocom.de>

If the FD is both readable and writable and the read callback closes the
connection (and frees the surrounding structure) we shouldn't call the
write callback (or check anything else in the read fd).

With this patch callback functions can return -EBADFD if they don't want
the FD to be handled any more.
---
 src/write_queue.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

Comments

Pablo Neira Ayuso June 10, 2014, 8:27 a.m. UTC | #1
On Tue, Jun 10, 2014 at 10:02:24AM +0200, Daniel Willmann wrote:
> From: Daniel Willmann <dwillmann@sysmocom.de>
> 
> If the FD is both readable and writable and the read callback closes the
> connection (and frees the surrounding structure) we shouldn't call the
> write callback (or check anything else in the read fd).
> 
> With this patch callback functions can return -EBADFD if they don't want
> the FD to be handled any more.
> ---
>  src/write_queue.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/src/write_queue.c b/src/write_queue.c
> index cef40f8..dcc0469 100644
> --- a/src/write_queue.c
> +++ b/src/write_queue.c
> @@ -21,8 +21,15 @@
>   *
>   */
>  
> +#include <errno.h>
>  #include <osmocom/core/write_queue.h>
>  
> +#define HANDLE_BAD_FD(rc, label) \
> +	do { \
> +		if (rc == -EBADFD) \
> +			goto label; \
> +	} while (0);

Do we really get anything good with this macro? This checking is only
required in three places in this patch.
Holger Freyther June 12, 2014, 10:19 a.m. UTC | #2
On Tue, Jun 10, 2014 at 10:27:52AM +0200, Pablo Neira Ayuso wrote:

> > +#define HANDLE_BAD_FD(rc, label) \
> > +	do { \
> > +		if (rc == -EBADFD) \
> > +			goto label; \
> > +	} while (0);
> 
> Do we really get anything good with this macro? This checking is only
> required in three places in this patch.

What is the argument against having this macro? readability?

Patch
diff mbox

diff --git a/src/write_queue.c b/src/write_queue.c
index cef40f8..dcc0469 100644
--- a/src/write_queue.c
+++ b/src/write_queue.c
@@ -21,8 +21,15 @@ 
  *
  */
 
+#include <errno.h>
 #include <osmocom/core/write_queue.h>
 
+#define HANDLE_BAD_FD(rc, label) \
+	do { \
+		if (rc == -EBADFD) \
+			goto label; \
+	} while (0);
+
 /*! \addtogroup write_queue
  *  @{
  */
@@ -39,14 +46,19 @@ 
 int osmo_wqueue_bfd_cb(struct osmo_fd *fd, unsigned int what)
 {
 	struct osmo_wqueue *queue;
+	int rc;
 
 	queue = container_of(fd, struct osmo_wqueue, bfd);
 
-	if (what & BSC_FD_READ)
-		queue->read_cb(fd);
+	if (what & BSC_FD_READ) {
+		rc = queue->read_cb(fd);
+		HANDLE_BAD_FD(rc, err_badfd);
+	}
 
-	if (what & BSC_FD_EXCEPT)
-		queue->except_cb(fd);
+	if (what & BSC_FD_EXCEPT) {
+		rc = queue->except_cb(fd);
+		HANDLE_BAD_FD(rc, err_badfd);
+	}
 
 	if (what & BSC_FD_WRITE) {
 		struct msgb *msg;
@@ -58,16 +70,21 @@  int osmo_wqueue_bfd_cb(struct osmo_fd *fd, unsigned int what)
 			--queue->current_length;
 
 			msg = msgb_dequeue(&queue->msg_queue);
-			queue->write_cb(fd, msg);
+			rc = queue->write_cb(fd, msg);
 			msgb_free(msg);
 
+			HANDLE_BAD_FD(rc, err_badfd);
+
 			if (!llist_empty(&queue->msg_queue))
 				fd->when |= BSC_FD_WRITE;
 		}
 	}
 
+err_badfd:
+	/* Return value is not checked in osmo_select_main() */
 	return 0;
 }
+#undef HANDLE_BAD_FD
 
 /*! \brief Initialize a \ref osmo_wqueue structure
  *  \param[in] queue Write queue to operate on