diff mbox

[1/2] write_queue: Avoid possible use-after-free if fd is read-/writable

Message ID b3f5691ae4dc33d9481275e3fbcc34aeef06ea34.1400677699.git.daniel@totalueberwachung.de
State Superseded
Headers show

Commit Message

Daniel Willmann May 21, 2014, 1:08 p.m. UTC 
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

Holger Freyther June 4, 2014, 3:58 p.m. UTC | #1 
On Wed, May 21, 2014 at 03:08:18PM +0200, Daniel Willmann wrote:

>  	return 0;
> +err_badfd:
> +	return rc;

hi, why do you want to change the return value here? Why is the
return 0 not good enough here?
Daniel Willmann June 10, 2014, 8:03 a.m. UTC | #2 
On Wed, 2014-06-04 at 17:58, Holger Hans Peter Freyther wrote:
> On Wed, May 21, 2014 at 03:08:18PM +0200, Daniel Willmann wrote:
> 
> >  	return 0;
> > +err_badfd:
> > +	return rc;
> 
> hi, why do you want to change the return value here? Why is the
> return 0 not good enough here?

You're right, it would be. In fact, the return value of this callback is
not checked in osmo_select_main() at all (and there doesn't seem to be
any use for it in that place either). I'll update the patch to return 0
in both cases.


Regards,
Daniel
diff mbox

Patch

diff --git a/src/write_queue.c b/src/write_queue.c
index cef40f8..d17493d 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;
 		}
 	}
 
 	return 0;
+err_badfd:
+	return rc;
 }
+#undef HANDLE_BAD_FD
 
 /*! \brief Initialize a \ref osmo_wqueue structure
  *  \param[in] queue Write queue to operate on