From owner-freebsd-hackers Sun Jun 23 17:54: 2 2002 Delivered-To: freebsd-hackers@freebsd.org Received: from flamingo.mail.pas.earthlink.net (flamingo.mail.pas.earthlink.net [207.217.120.232]) by hub.freebsd.org (Postfix) with ESMTP id D213F37B401 for ; Sun, 23 Jun 2002 17:53:57 -0700 (PDT) Received: from pool0336.cvx22-bradley.dialup.earthlink.net ([209.179.199.81] helo=mindspring.com) by flamingo.mail.pas.earthlink.net with esmtp (Exim 3.33 #2) id 17MI7A-0007Yz-00; Sun, 23 Jun 2002 17:53:56 -0700 Message-ID: <3D166D7D.6CD9CC90@mindspring.com> Date: Sun, 23 Jun 2002 17:53:17 -0700 From: Terry Lambert X-Mailer: Mozilla 4.7 [en]C-CCK-MCD {Sony} (Win98; U) X-Accept-Language: en MIME-Version: 1.0 To: Jonathan Lemon Cc: dillon@apollo.backplane.com, hackers@freebsd.org Subject: Re: Bug in wakeup() (stable and current) ? References: <200206232158.g5NLw9c49030@prism.flugsvamp.com> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Sender: owner-freebsd-hackers@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Jonathan Lemon wrote: > > Look at the code carefully. It's *removing* the element from the list, > > the conditionally restarting rather then removing the element from the > > list and unconditionally restarting. The only reason it works at all > > is because sys/queue.h does not clear out the pointers in the node > > that was just removed. The code is just plain wrong, though, because > > the queue mechanisms make no such (documented) guarentee. > > Looks like the original damage happened in r1.21, where the temporary > variable (used to hold the next item on the list) was replaced by a > dereference through the pointer of the item that was just removed. > > The code works simply because it relies TAILQ_REMOVE() not changing > the tqe_next pointer. I suppose that this should either be documented, > or the loop changed back to use a temp variable: > > for (td = TAILQ_FIRST(qp); td != NULL; td = tdq) { > tdq = TAILQ_NEXT(td, td_slpq); > ... > } Too bad the first argument to TAILQ_FOREACH isn't a pointer to a pointer, instead of just a pointer. I guess the double dereference is considered "too expensive". It's a bummer that you can't safely use TAILQ_REMOVE inside the TAILQ_FOREACH. FWIW, this looks like a general bug in queue.h for all queue and list types, so there are probably other places it would be an issue. 8-(. -- Terry To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message