First Last Prev Next    No search results available
Details
: Possible crashes due to bad code in send_connection_close...
Bug#: 1614
: TAO
: ORB
Status: RESOLVED
Resolution: FIXED
: All
: All
: 1.3.4
: P3
: normal
: ---

:
:
:
:
  Show dependency tree - Show dependency graph
People
Reporter: Carlos O'Ryan <coryan@atdesk.com>
Assigned To: DOC Center Support List (internal) <tao-support@dre.vanderbilt.edu>

Attachments


Note

You need to log in before you can comment on or make changes to this bug.

Related actions
Votes: 0


Description:   Opened: 2003-10-15 11:04
When a connection is closed all the outgoing messages are marked as "connection
closed" and removed from Transport queue.  However, the loop reads:

  while (this->head_ != 0)
    {
      TAO_Queued_Message *i = this->head_;
      [snip]
      this->head_ = i->next();
      [snip]
    }

this loop breaks all sorts of invariants in the doubly-linked list used for the
outgoing queue.  For example, at the end of the loop tail_ is not null, but
head_ is (either both are null or none of them is.)  Furthermore, the first
element in the doubly-linked list is not pointing to the head.

I suggest that we apply the following patch:

--- TAO/tao/Transport.cpp	10 Aug 2003 20:23:40 -0000	1.1.1.6
+++ TAO/tao/Transport.cpp	15 Oct 2003 15:01:55 -0000
@@ -1109,7 +1109,7 @@
       //    CloseConnection message was successfully received.
       i->state_changed (TAO_LF_Event::LFS_CONNECTION_CLOSED);
 
-      this->head_ = i->next ();
+      i->remove_from_list(this->head_, this->tail_);
 
       i->destroy ();
     }
------- Comment #1 From Nanbor Wang 2003-10-15 11:24:25 -------
Fixed. Thanks very much!

Wed Oct 15 11:22:01 2003  Balachandran Natarajan  <bala@dre.vanderbilt.edu>
------- Comment #2 From Carlos O'Ryan 2003-10-22 07:21:41 -------
Sorry, my suggestion was wrong.  The problem is that the TAO_Queue_Message
class
is brittle.  The remove_from_list() operation cannot be called two times.  With
my patches this can happen often (check Transport::send_synchronous_message)

My new suggestion is two fold.  First change the TAO_Queued_Message to be safe
in this regard:

Index: tao/Queued_Message.cpp
===================================================================
RCS file: /home/src/ACE_wrappers/TAO/tao/Queued_Message.cpp,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 Queued_Message.cpp
--- tao/Queued_Message.cpp      13 Dec 2002 14:35:12 -0000      1.1.1.2
+++ tao/Queued_Message.cpp      22 Oct 2003 12:16:44 -0000
@@ -3,6 +3,8 @@

 #include "Queued_Message.h"

+#include "ace/Log_Msg.h"
+
 #if !defined (__ACE_INLINE__)
 # include "Queued_Message.inl"
 #endif /* __ACE_INLINE__ */
@@ -28,16 +30,13 @@
 {
   if (this->prev_ != 0)
     this->prev_->next_ = this->next_;
-  else
+  else if(head == this)
     head = this->next_;

   if (this->next_ != 0)
     this->next_->prev_ = this->prev_;
-  else
+  else if(tail == this)
     tail = this->prev_;
-
-  this->next_ = 0;
-  this->prev_ = 0;
 }

 void
------------------------------------------------------

Next change the TAO_Transport class to not violate invariants:
Index: tao/Transport.cpp
===================================================================
RCS file: /home/src/ACE_wrappers/TAO/tao/Transport.cpp,v
retrieving revision 1.1.1.6
diff -u -r1.1.1.6 Transport.cpp
--- tao/Transport.cpp   10 Aug 2003 20:23:40 -0000      1.1.1.6
+++ tao/Transport.cpp   22 Oct 2003 12:17:21 -0000
@@ -534,10 +534,11 @@
   }
   if (result == -1)
     {
+      bool at_head = (this->head_ == &synch_message);
       synch_message.remove_from_list (this->head_, this->tail_);
       if (errno == ETIME)
         {
-          if (this->head_ == &synch_message)
+          if (at_head)
             {
               // This is a timeout, there is only one nasty case: the
               // message has been partially sent!  We simply cannot take
@@ -553,7 +554,6 @@
               // nightmare.
               //

-              synch_message.remove_from_list (this->head_, this->tail_);
               TAO_Queued_Message *queued_message = 0;
               ACE_NEW_RETURN (queued_message,
                               TAO_Asynch_Queued_Message (
@@ -1101,17 +1101,18 @@
 void
 TAO_Transport::send_connection_closed_notifications_i (void)
 {
-  while (this->head_ != 0)
+  for(TAO_Queued_Message *i = this->head_; i != 0;)
     {
-      TAO_Queued_Message *i = this->head_;
+      TAO_Queued_Message * next = i->next();

       // @@ This is a good point to insert a flag to indicate that a
       //    CloseConnection message was successfully received.
+      //    -- I think the previous comment is wrong.  The 
       i->state_changed (TAO_LF_Event::LFS_CONNECTION_CLOSED);
-
-      this->head_ = i->next ();
-
+      i->remove_from_list(this->head_, this->tail_);
       i->destroy ();
+
+      i = next;
     }

   this->tms ()->connection_closed ();
---------------------------------------------------------------------
------- Comment #3 From Carlos O'Ryan 2003-11-13 20:03:58 -------
Fixed, please check the following ChangeLog entry for details:
Thu Nov 13 21:02:31 2003  Carlos O'Ryan  <coryan@atdesk.com>

First Last Prev Next    No search results available