ansaurus

Question

C code - need to clarify the effectiveness.

Answer 1

+1 A:

My feeling is that this code is very brittle. It may well work when given good input (I don't propose to desk check the thing for you) but if given some incorrect inputs it will either crash and burn or give misleading results.

Have you tested for unexpected inputs? For example:

Suppose i_balance_info is null?
Suppose i_balance_info is ""?
Suppose there are fewer than 8 items in the input string, what will this line of code do?
```
memcpy(o_balance_sign,str[7],1);
```
Suppose that that the item in str[3] is less than 16 chars long, what will this line of code do?
```
memcpy(o_balance_change,str[3],16);
```

My approach to writing such code would be to protect against all such eventualities. At the very least I would add ASSERT() statements, I would usually write explicit input validation and return errors when it's bad. The problem here is that the interface does not seem to allow for any possibility that there might be bad input.

djna 2009-10-09 08:00:55

@dgna,first thing is i have a confirmation that i will never receive less than 8 fields ina single bucket. only thing is ther can be acse where i may not receive 20 buckets some times.secondly balnace info will never be null .since i am passing it from some other function after checking its not null

Vijay Sarathi 2009-10-09 09:53:31

The nature of code is that it gets reused in new contexts, and under new assumptions. Defensive coding protects you from changes (and bugs) in the thing calling you. If you are concerned with performance overheads, then use ASSERT(), it can be used during development and compiled out for production

djna 2009-10-09 11:36:02

Answer 2

A:

I had a hard time reading your code but FWIW I've added some comments, HTH:

// do shorter functions, long functions are harder to follow and make errors harder to spot
// document all your variables, at the very least your function parameters
// also what the function is suppose to do and what it expects as input
int CMI9_auxc_parse_balance_info
(
  char *i_balance_info,
  char *i_use_balance_ind,
  char *o_balance,
  char *o_balance_change,
  char *o_balance_sign
)
{
  char *balance_id[MAX_BUCKETS] = {NULL};
  char balance_info[BALANCE_INFO_FIELD_MAX_LENTH] = {0};
  char *str[160] = {NULL};
  int i=0,j=0,b_id=0,b_ind=0,bc_ind=0,bs_ind=0,rc;
  int total_bukets=0; // good practice to initialize all variables

  //
  // check for null pointers in your arguments, and do sanity checks for any
  // calculations
  // also move variable declarations to just before they are needed
  //

  memset(balance_info,' ',BALANCE_INFO_FIELD_MAX_LENTH);
  memcpy(balance_info,i_balance_info,BALANCE_INFO_FIELD_MAX_LENTH);
  //balance_info[BALANCE_INFO_FIELD_MAX_LENTH]='\0';  // should be BALANCE_INFO_FIELD_MAX_LENTH-1

  char *pch = strtok (balance_info,"*"); // this will potentially crash since no ending \0

  while (pch != NULL && i < 160)
  {
    str[i]=(char*)malloc(strlen(pch) + 1);
    strcpy(str[i],pch);
    pch = strtok (NULL, "*");
    i++;
  }
  total_bukets  = i/8  ;
  // you have declared char*str[160] check if enough b_id < 160
  // asserts are helpful if nothing else assert( b_id < 160 );
  for (j=0;str[b_id]!=NULL,j<total_bukets;j++)
  {
    balance_id[j]=str[b_id];
    b_id=b_id+8;
  }
  // don't use memcmp, if ('y'==i_use_balance_ind[0]) is better
  if (!memcmp(i_use_balance_ind,"Y",1))
  {
    // atoi needs balance_id str to end with \0 has it?
    if (atoi(balance_id[0])==1)
    {
      // length assumptions and memcpy when its only one byte
      memcpy(o_balance,str[2],16);
      memcpy(o_balance_change,str[3],16);
      memcpy(o_balance_sign,str[7],1);
      for(i=0;i<160;i++)
        free(str[i]);
      return 1;
    }
    else
    {
      for(i=0;i<160;i++)
        free(str[i]);
      return 0;
    }
  }
  // if ('N'==i_use_balance_ind[0]) 
  else if (!memcmp(i_use_balance_ind,"N",1))
  {
    // here I get a headache, this looks just at first glance risky. 
    for (j=1;balance_id[j]!=NULL,j<MAX_BUCKETS;j++)
    {
      b_ind=(j*8)+2;
      bc_ind=(j*8)+3;
      bs_ind=(j*8)+7;
      if (atoi(balance_id[j])!=1 && atoi( str[bc_ind] )!=0)
      {
        // length assumptions and memcpy when its only one byte
        // here u assume strlen(str[b_ind])>15 including \0
        memcpy(o_balance,str[b_ind],16);
        // here u assume strlen(str[bc_ind])>15 including \0
        memcpy(o_balance_change,str[bc_ind],16);
        // here, besides length assumption you could use a simple assignment
        // since its one byte
        memcpy(o_balance_sign,str[bs_ind],1);
        // a common practice is to set pointers that are freed to NULL.
        // maybe not necessary here since u return
        for(i=0;i<160;i++)
          free(str[i]);
        return 1;
      }
    }
    // suggestion do one function that frees your pointers to avoid dupl
    for(i=0;i<160;i++)
      free(str[i]);
    return 0;
  }
  for(i=0;i<160;i++)
    free(str[i]);
  return 0;
}

A helpful technique when you want to access offsets in an array is to create a struct that maps the memory layout. Then you cast your pointer to a pointer of the struct and use the struct members to extract information instead of your various memcpy's

I would also suggest you reconsider your parameters to the function in general, if you place every of them in a struct you have better control and makes the function more readable e.g.

int foo( input* inbalance, output* outbalance )

(or whatever it is you are trying to do)

Anders K. 2009-10-09 08:04:43

ansaurus

tags:

views:

answers:

C code - need to clarify the effectiveness.

related questions