[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-ha-dev
Subject:    [Linux-ha-dev] Re: Messaging patch
From:       Alan Robertson <alanr () unix ! sh>
Date:       2006-05-11 12:52:01
Message-ID: 44633371.8050701 () unix ! sh
[Download RAW message or body]

Andrew Beekhof wrote:
> Coverity started complaining about a bounds overrun... can someone check 
> this patch is ok?
> 
> Index: heartbeat/heartbeat.c
> ===================================================================
> RCS file: /home/cvs/linux-ha/linux-ha/heartbeat/heartbeat.c,v
> retrieving revision 1.506
> diff -u -r1.506 heartbeat.c
> --- heartbeat/heartbeat.c 26 Apr 2006 03:42:07 -0000 1.506
> +++ heartbeat/heartbeat.c 11 May 2006 09:22:43 -0000
> @@ -5975,8 +5975,8 @@
> longclock_t last_rexmit;
> size_t len;
> - if (msgslot < 0) {
> - msgslot = MAXMSGHIST;
> + if (msgslot < 0 || msgslot >= MAXMSGHIST) {
> + msgslot = MAXMSGHIST-1;
> }
> if (hist->msgq[msgslot] == NULL) {
> continue;

This looks like a real bug.  One that could in theory cause a crash.

I think this patch might cause it to loop infinitely if
   firstslot == MAXMSGHIST-1.

Let's see...

We decrement and decrement, and decrement, then the index goes negative, 
and we then reset it to MAXMSGHIST-1.

We go through one of the continue statements, and decrement msgslot 
again.  It is now MAXMSGHIST-2.  We can now loop forever...

So, I don't think that this is quite right yet...

This can happen if the sequence number we're being asked to rexmit isn't 
found at all (it's too old, or bogus or whatever).


Here's a similar patch, but one that moves one of the tests outside the 
loop, and which hopefully doesn't loop...


Index: heartbeat.c
===================================================================
RCS file: /home/cvs/linux-ha/linux-ha/heartbeat/heartbeat.c,v
retrieving revision 1.506
diff -u -r1.506 heartbeat.c
--- heartbeat.c 26 Apr 2006 03:42:07 -0000      1.506
+++ heartbeat.c 11 May 2006 12:49:22 -0000
@@ -5911,10 +5911,16 @@
         struct node_info* fromnode = NULL;

         if (fromnodename == NULL){
-               cl_log(LOG_ERR, "process_rexmit: "
-                      "from node not found in the message");
+               cl_log(LOG_ERR, "process_rexmit"
+               ": from node not found in the message");
                 return;
         }
+       if (firstslot >= MAXMSGHIST) {
+               cl_log(LOG_ERR, "process_rexmit"
+               ": firstslot out of range [%d]"
+               ,       );
+               hist->lastmsg = firstslot = MAXMSGHIST-1;
+       }

         fromnode = lookup_tables(fromnodename, NULL);
         if (fromnode == NULL){
@@ -5976,7 +5982,11 @@
                         size_t          len;

                         if (msgslot < 0) {
-                               msgslot = MAXMSGHIST;
+                               if (firstslot == MAXMSGHIST-1) {
+                                       /* We've wrapped around */
+                                       break;
+                               }
+                               msgslot = MAXMSGHIST-1;
                         }
                         if (hist->msgq[msgslot] == NULL) {
                                 continue;


-- 
     Alan Robertson <alanr@unix.sh>

"Openness is the foundation and preservative of friendship...  Let me 
claim from you at all times your undisguised opinions." - William 
Wilberforce
_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/
[prev in list] [next in list] [prev in thread] [next in thread]