strcmp() fooling me? :o

[Ron Eggler]

Hi,

I got a problem: I have a function that allocates new memory to hold another
element of my struct array if it's not existing already.

/** My Structures **/
typedef struct {
        char BusID[6];
        int SeqNr;
        unsigned short Msg;
        } MsgStruct;
typedef struct{
        unsigned char  app;
        unsigned char  pri;
        unsigned char  act;
        int            seq;
        char   id[6];   
}io_t;
/* My function */
int CheckStruct(MsgStruct **SumStruct_ptr, io_t *CurrentBus)
{       
  int i=0;
  if (StructCount==0){
/* allocating memory to hold the 1st element */
  }
  /* loop thru all elements in the struct array and check if
     busID and seq# matches with an existing element
     if not, create new element */
  for (i=0; i < StructCount; i++){
    if((strcmp((*SumStruct_ptr)[i].BusID, CurrentBus->id)==0) &&
      ((*SumStruct_ptr)[i].SeqNr== CurrentBus->seq)){
      return i;
    } /*if we're at the end of the array and nothing matched -  extend
memory*/ 
    else if( i == StructCount - 1){
      (*SumStruct_ptr) = realloc((*SumStruct_ptr),
      (StructCount+1)*sizeof(MsgStruct ));
      StructCount++;
      strncpy((*SumStruct_ptr)[i+1].BusID, CurrentBus->id, 6);
      (*SumStruct_ptr)[i+1].SeqNr=CurrentBus->seq;
      (*SumStruct_ptr)[i+1].Msg=0;
      prs_log(LOG_NOTICE,"***CheckStruct() - Added new element[%d]: BusID:%s
seqNr:%d - Count: %d", StructCount-1, (*SumStruct_ptr)[i+1].BusID,
(*SumStruct_ptr)[i+1].SeqNr, StructCount);
      return i+1;
    }
  }
}
MsgStruct *
ClearElement(MsgStruct ** SumStruct_ptr,
             int         Position)
{
    /* Check for invalid 'Position' value */
    prs_log(LOG_NOTICE,"***ClearElement() - Will remove element [%d]: BusID
%s seqNr:%d - Count: %d", Position,(*SumStruct_ptr)[Position].BusID,
(*SumStruct_ptr)[Position].SeqNr, StructCount);
    if ( Position>= StructCount )
         return (*SumStruct_ptr);

    /* If 'Position' doesn't index the very last element move all the
       ones following it to a position with an index smaller by 1,
       thereby overwriting the element at 'Position' */

    if ( Position != StructCount - 1 )
        memmove( SumStruct_ptr + Position, SumStruct_ptr + Position + 1,
                 ( StructCount - Position - 1 ) * sizeof *SumStruct_ptr );

    /* The last element isn't used anymore, so give back the memory
       used for it. */
    StructCount--;
    return (*SumStruct_ptr) = realloc( (*SumStruct_ptr), StructCount *
sizeof *SumStruct_ptr );

And this is what happened (copy/paste from my syslog - where prs_log() is
writing to):
"CheckStruct() - Added new element[2]: BusID:T9998 seqNr:9 - Count: 3"
"ClearElement() - Will remove element [0]: BusID:T1111 seqNr:627 - Count: 2"
"CheckStruct() - Added new element[2]: BusID:T9998 seqNr:9 - Count: 3"
Now... Why did it create a second entry tor T9998, SEQ#9?
Shouldn't it have recognized the one that's there already? Held on position
1 after the element 0 has been cleared or do you see some mistake in my
clearing function? I get pretty stuck here...
Thanks for everybody's hints and suggestions!
Ron

 

[christian.bau]

Thanks for everybody's hints and suggestions!

I recommend that you move *SumStruct_ptr into a local variable as
early as possible. This will make your code much more readable and the
bug in your code very obvious. Also, it seems quite weird that you
pass a pointer that is modified by address, and use another related
variable as a global variable. If you had a single struct containing
the pointer and the count, the code would be much cleaner and again,
the error would become obvious.

 

[christian.bau]

Another recommendation: Your combination of using strncpy to fill an
array of char and strcmp to compare the contents is dangerous. Either
your strings are limited to five characters and fit, then use strcpy.
Or they can be longer than five characters, then the call to strcmp is
dangerous and can give the wrong result.

 

[Ron Eggler]

[...]
MsgStruct *
ClearElement(MsgStruct ** SumStruct_ptr,
int Position)
{
[...]
if ( Position != StructCount - 1 )
memmove( SumStruct_ptr + Position, SumStruct_ptr + Position + 1,
( StructCount - Position - 1 ) * sizeof *SumStruct_ptr
);

You've forgotten one level of indirection:

memmove( (*SumStruct_ptr) + Position,
(*SumStruct_ptr) + Position + 1,
(StructCount - Position - 1) * sizeof **SumStruct_ptr);

Exactly, great - thank you

 

[Ron Eggler]

I recommend that you move *SumStruct_ptr into a local variable as
early as possible. This will make your code much more readable and the
bug in your code very obvious. Also, it seems quite weird that you
pass a pointer that is modified by address, and use another related
variable as a global variable. If you had a single struct containing
the pointer and the count, the code would be much cleaner and again,
the error would become obvious.

Well the reason for me using a double pointer is, because i want to use the
array elements from my main loop and this specific struct variable (in the
array) should keep its value from time of allocation until it is destroyed
again in ClearElement().
Does this make more sense now?

 

[Ron Eggler]

Another recommendation: Your combination of using strncpy to fill an
array of char and strcmp to compare the contents is dangerous. Either
your strings are limited to five characters and fit, then use strcpy.
Or they can be longer than five characters, then the call to strcmp is
dangerous and can give the wrong result.

Yes i know this but BusID will always be 5 characters only. It'll always be
in the form "X1234" so I should be safe with strncpy() and strcmp(). But
thanks for pointing this out!

Ron

 

[Keith Thompson]

Yes i know this but BusID will always be 5 characters only. It'll always be
in the form "X1234" so I should be safe with strncpy() and strcmp(). But
thanks for pointing this out!

strncpy() is rarely what you want. It isn't *really* a string
function. A string is an arbitrary sequence of characters, terminated
by and including a trailing '\0'. The data structure that strncpy()
deals with (at least in its target) is a fixed-length sequence of
characters ending in zero or more '\0's.

You probably might as well use strcpy() (or perhaps memcpy()).

 

[christian.bau]

Well the reason for me using a double pointer is, because i want to use the
array elements from my main loop and this specific struct variable (in the
array) should keep its value from time of allocation until it is destroyed
again in ClearElement().
Does this make more sense now?

No. Your data is based on two values: One is a variable "StructCount",
one is a malloc'd pointer to that number of MsgStruct structures. This
two values are logically inseparable, but you are separating them.
Why? You not only keep them separate, you pass them to your two
functions in different ways: One as a global variable (apparently),
and one by passing it's address. Either use two globals, or pass the
address of StructCount, or put StructCount and the pointer together
into one struct.