[Freeipmi-devel] fiid_obj_get bugs

[Albert Chu]

Hi all,

I found several bugs in fiid_obj_get() while hunting down the cause for
why fiid_obj_get() didn't function properly for a field of length 64.  I
believe several corner cases were never hit b/c fiid_obj_get() wasn't
used for anything >= 64 bits.

Since this is a pretty critical area for fiid, I wanted to post to get
comments before I commit.  Here are the bugs I found:

1) If the field was > 64 bits (i.e. > 8 bytes), the function would
return invalid values.  Due to invalid bit shifting (i.e. bit shift 72
bits to the right), zeroes would get populated into the 'val' return
value.  The fix is to of course limit fiid_obj_get to fields <= 64 bits.

2) The main loop was typically iterated 1 extra time.  If the field
length was greater than 56 bits (i.e. > 7 bytes), the loop would iterate
9 times rather than 8.  The additional iteration would populate in a 0
into the last byte of the 'val' return value, destroying some data.

3) The end bit position was computed improperly on corner cases.  It
seems the common situation was when a field was a multiple of 8, so the
"% 8" computation earlier in the code messed things up.  It seems that
this didn't actually cause any issues due to the additional loop
iteration up. 

Here are my changes.

RCS file: /cvsroot/freeipmi/freeipmi/libfreeipmi/src/fiid.c,v
retrieving revision 1.9
diff -p -u -b -r1.9 fiid.c
--- fiid.c      16 Dec 2005 08:48:40 -0000      1.9
+++ fiid.c      14 Jan 2006 02:30:22 -0000
@@ -384,25 +384,36 @@ fiid_obj_get (fiid_obj_t obj,
   field_len = fiid_obj_field_start_end (tmpl, field, &start_bit_pos,
&end_bit_pos);
   ERR (field_len != -1);

+  if (field_len > 64)
+    field_len = 64;
+
   byte_pos = start_bit_pos / 8;

   /* in byte_pos, start_bit_pos is  */
   start_bit_in_byte_pos = start_bit_pos - (byte_pos * 8);
-  bytes_used = 1;

   /*   and it spans into... */
   if (start_bit_in_byte_pos + field_len > 8)
     {
-      field_len -= start_bit_in_byte_pos;
-      bytes_used += (field_len / 8);
-      field_len %= 8;
-      if (field_len != 0)
-       {
+      int field_len_temp = field_len;
+
+      if (start_bit_in_byte_pos)
+       bytes_used++;
+
+      field_len_temp -= start_bit_in_byte_pos;
+
+      bytes_used += (field_len_temp / 8);
+
+      field_len_temp -= (bytes_used * 8);
+
+      if (field_len_temp)
          bytes_used++;
-       }
     }
   else
+    {
     end_bit_in_byte_pos = start_bit_in_byte_pos + field_len;
+      bytes_used = 1;
+    }

   if (bytes_used > 1)
     {
@@ -418,12 +429,12 @@ fiid_obj_get (fiid_obj_t obj,
            end_val_pos = 8 - start_bit_in_byte_pos;
          else
            if (i == (bytes_used - 1))
-             end_val_pos += field_len;
+              end_val_pos += (field_len - (bytes_used - 1) * 8 -
start_bit_in_byte_pos);
            else
              end_val_pos += 8;

          if (i == (bytes_used - 1))
-           end_bit_in_byte_pos = field_len;
+           end_bit_in_byte_pos = (field_len - (bytes_used - 1) * 8 -
start_bit_in_byte_pos);
          else
            end_bit_in_byte_pos = 8;


Al

--
Albert Chu
address@hidden
925-422-5311
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory