kern/119307: TRASHIT macro blasts list header if REMOVE invoked with header ref

[Doug Moore]

>Number:         119307
>Category:       kern
>Synopsis:       TRASHIT macro blasts list header if REMOVE invoked with header ref
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Jan 03 16:30:03 UTC 2008
>Closed-Date:
>Last-Modified:
>Originator:     Doug Moore <dougm at ironport.com>
>Release:        FreeBSD 7.0-CURRENT i386
>Organization:
>Environment:
System: FreeBSD adsl-216-63-78-19.dsl.hstntx.swbell.net 7.0-CURRENT FreeBSD 7.0-CURRENT #5: Sat Sep 15 00:42:55 CDT 2007 root at adsl-216-63-78-19.dsl.hstntx.swbell.net:/usr/src/sys/i386/compile/ATHEROS i386


	
>Description:
	A line like
	TAILQ_REMOVE(&a->tailq, a->tailq.tqh_first, next);
	will corrupt the tailq in question if QUEUE_MACRO_DEBUG is on,
	since after the first tailq item is removed, it is the *new*
	first tailq item that gets its prev and next pointers trashed.
	With QUEUE_MACRO_DEBUG off, it removes the first tailq as
	expected.  Therefore, turning on QUEUE_MACRO_DEBUG corrupts
	the tailq.
>How-To-Repeat:
	Write a self-referential invocation of a removal from tailq,
	list, etc, and add a couple of items to the list before
	invoking that removal.
>Fix:
	Use typeof to bind the value of the to-be-removed element to a
	name, and use that name in the pointer trashing and other
	debugging code.  If typeof is forbidden, then it would seem to
	require adding type arguments to several macros, a bad thing.

--- queue.h.diff begins here ---
--- queue.h	2008-01-02 22:53:54.000000000 -0600
+++ queue.h.new	2008-01-02 23:02:28.000000000 -0600
@@ -188,6 +188,7 @@ struct {								\
 #define	SLIST_NEXT(elm, field)	((elm)->field.sle_next)
 
 #define	SLIST_REMOVE(head, elm, type, field) do {			\
+	typeof (elm) oldelm = (elm);					\
 	if (SLIST_FIRST((head)) == (elm)) {				\
 		SLIST_REMOVE_HEAD((head), field);			\
 	}								\
@@ -198,7 +199,7 @@ struct {								\
 		SLIST_NEXT(curelm, field) =				\
 		    SLIST_NEXT(SLIST_NEXT(curelm, field), field);	\
 	}								\
-	TRASHIT((elm)->field.sle_next);					\
+	TRASHIT(oldelm->field.sle_next);					\
 } while (0)
 
 #define	SLIST_REMOVE_HEAD(head, field) do {				\
@@ -280,6 +281,7 @@ struct {								\
 #define	STAILQ_NEXT(elm, field)	((elm)->field.stqe_next)
 
 #define	STAILQ_REMOVE(head, elm, type, field) do {			\
+	typeof (elm) oldelm = (elm);					\
 	if (STAILQ_FIRST((head)) == (elm)) {				\
 		STAILQ_REMOVE_HEAD((head), field);			\
 	}								\
@@ -291,7 +293,7 @@ struct {								\
 		     STAILQ_NEXT(STAILQ_NEXT(curelm, field), field)) == NULL)\
 			(head)->stqh_last = &STAILQ_NEXT((curelm), field);\
 	}								\
-	TRASHIT((elm)->field.stqe_next);				\
+	TRASHIT(oldelm->field.stqe_next);				\
 } while (0)
 
 #define	STAILQ_REMOVE_HEAD(head, field) do {				\
@@ -392,14 +394,15 @@ struct {								\
 #define	LIST_NEXT(elm, field)	((elm)->field.le_next)
 
 #define	LIST_REMOVE(elm, field) do {					\
+	typeof (elm) oldelm = (elm);					\
 	QMD_LIST_CHECK_NEXT(elm, field);				\
 	QMD_LIST_CHECK_PREV(elm, field);				\
 	if (LIST_NEXT((elm), field) != NULL)				\
 		LIST_NEXT((elm), field)->field.le_prev = 		\
 		    (elm)->field.le_prev;				\
 	*(elm)->field.le_prev = LIST_NEXT((elm), field);		\
-	TRASHIT((elm)->field.le_next);					\
-	TRASHIT((elm)->field.le_prev);					\
+	TRASHIT(oldelm->field.le_next);					\
+	TRASHIT(oldelm->field.le_prev);					\
 } while (0)
 
 /*
@@ -554,6 +557,7 @@ struct {								\
 	(*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
 
 #define	TAILQ_REMOVE(head, elm, field) do {				\
+	typeof (elm) oldelm = (elm);					\
 	QMD_TAILQ_CHECK_NEXT(elm, field);				\
 	QMD_TAILQ_CHECK_PREV(elm, field);				\
 	if ((TAILQ_NEXT((elm), field)) != NULL)				\
@@ -564,9 +568,9 @@ struct {								\
 		QMD_TRACE_HEAD(head);					\
 	}								\
 	*(elm)->field.tqe_prev = TAILQ_NEXT((elm), field);		\
-	TRASHIT((elm)->field.tqe_next);					\
-	TRASHIT((elm)->field.tqe_prev);					\
-	QMD_TRACE_ELEM(&(elm)->field);					\
+	TRASHIT(oldelm->field.tqe_next);				\
+	TRASHIT(oldelm->field.tqe_prev);				\
+	QMD_TRACE_ELEM(&oldelm->field);					\
 } while (0)
 
 
--- queue.h.diff ends here ---


>Release-Note:
>Audit-Trail:
>Unformatted: