Bug in void AddMacroValue(name,value)

[GERMA Denis]

Hi,

While tying to understand the variable concept in cfengine,
I think I have found a bug in the function AddMacroValue(name,value)

File macro.c
Cfengine 1.6.3

The bug is revealed if :
1- you insert a variable which has a collision in the hash table.
    I.E. is inserted in slot  Hash(name) + 2 % .... because the slot
Hash(name) + 1 % ....and
    Hash(name)  were occupied.
2- you try to reinsert the same variable, the algorithme will not see
that the variable is
allready defined in Hash(name) + 2 % ...., and insert it again, for
example  in slot Hash(name) + 3 % ...

The retrived value will then be the first value set, and no warning
would have been issued.

--------------------------------------------------------------------------

If  I have correctly understand your implementation of this hash algo. :

On collision you
1- compare the new entry with Hash(name) slot
2- find a free slot to insert the variable.

You do not compare the new entry to the entries
following Hash(name) slot which could be
a slot where you previously entered the same variable as the new entry.

------------------------------------------------------------
The current algorithme is :
=============
slot = Hash(name);

if (HASH[slot] != 0)
   {
   Debug("Macro Collision!\n");
   if (CompareMacro(name,HASH[slot]) == 0)
      {
      sprintf(VBUFF,"Redefinition of macro %s=%s",name,exp);
      Warning(VBUFF);
      free(HASH[slot]);
      HASH[slot] = sp;
      return;
      }

   while ((HASH[++slot % hashtablesize] != 0))
      {
    if (slot == hashtablesize-1)
         {
         slot = 0;
         }
      if (slot == Hash(name))
         {
         FatalError("AddMacroValue - internal error #1");
         }
      }
   }



==============
Should be (or something similar) :
=============

slot = Hash(name);

if (HASH[slot] != 0)
   {
   Debug("Macro Collision!\n");
   if (CompareMacro(name,HASH[slot]) == 0)
      {
      sprintf(VBUFF,"Redefinition of macro %s=%s",name,exp);
      Warning(VBUFF);
      free(HASH[slot]);
      HASH[slot] = sp;
      return;
      }

   while ((HASH[++slot % hashtablesize] != 0))
      {
       if (slot == hashtablesize-1)
         {
         slot = 0;
         }
      if (slot == Hash(name))
         {
         FatalError("AddMacroValue - internal error #1");
         }
        if (CompareMacro(name,HASH[slot]) == 0)
        {
          sprintf(VBUFF,"Redefinition of macro %s=%s",name,exp);
          Warning(VBUFF);
          free(HASH[slot]);
          HASH[slot] = sp;
          return;
        }
    }
   }


=================
I Suggest (not tested) :
==================

slot = Hash(name);

if (HASH[slot] != 0)
   {
    int slot_ref = slot; /* C-ISO syntaxe  Some compilator may prefer
you define this variable at the begining of the function ? */
   Debug("Macro Collision!\n");
   while ((HASH[slot] != 0))
      {
        slot = (slot+1) % hashtablesize;
       if (CompareMacro(name,HASH[slot]) == 0)
        {
          sprintf(VBUFF,"Redefinition of macro %s=%s",name,exp);
          Warning(VBUFF);
          free(HASH[slot]);
          HASH[slot] = sp;
          return;
        }
      if (slot == slot_ref)
         {
             FatalError("AddMacroValue - internal error #1");
         }
      }
   }



Thanks for all.
D.Germa