C代码 - 需要澄清的有效性

Question

您好我已经编写了一个基于需求的代码。

（field1_6）（field2_30）（field3_16）（field4_16）（field5_1）（field6_6）（field7_2）（field8_1） ..... 这是一个斗式（8场）数据的。我们将一次收到20个桶，共计160个字段。 我需要根据预定义的条件取field3，field7 & fields8的值。 如果输入参数是N，则从第一个桶取三个字段，如果是Y，我需要 从除第一个桶以外的任何其他桶取得三个字段。 如果argumnet是Y，那么我需要一个接一个地扫描所有20个桶并检查 桶的第一个字段不等于0，如果它是true，则获取该桶的三个字段并退出。 我已经写了代码，它也工作得很好..但不那么自信，它是有效的。 我恐怕有一段时间会崩溃。请在下面提示是代码。

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 *pch = NULL; 
    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 ; 
    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'; 
    pch = strtok (balance_info,"*"); 
    while (pch != NULL && i < 160) 
    { 
    str[i]=(char*)malloc(strlen(pch) + 1); 
    strcpy(str[i],pch); 
    pch = strtok (NULL, "*"); 
    i++; 
    } 
total_bukets = i/8 ; 
    for (j=0;str[b_id]!=NULL,j<total_bukets;j++) 
    { 
    balance_id[j]=str[b_id]; 
    b_id=b_id+8; 
    } 
    if (!memcmp(i_use_balance_ind,"Y",1)) 
    { 
    if (atoi(balance_id[0])==1) 
    { 
     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; 
    } 
    } 
    else if (!memcmp(i_use_balance_ind,"N",1)) 
    { 
     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) 
     { 
     memcpy(o_balance,str[b_ind],16); 
     memcpy(o_balance_change,str[bc_ind],16); 
     memcpy(o_balance_sign,str[bs_ind],1); 
     for(i=0;i<160;i++) 
     free(str[i]); 
     return 1; 
     } 
     } 
    for(i=0;i<160;i++) 
    free(str[i]); 
    return 0; 
    } 
for(i=0;i<160;i++) 
free(str[i]); 
return 0; 
} 

Answer 1

我做了一个艰难的时间阅读你的代码，但FWIW我已经添加了一些意见，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; 
} 

，当你要访问数组中的偏移一个有用的方法是创建一个映射的一个结构内存布局。然后你将指针指向结构体的指针，并使用struct成员来提取信息，而不是你的各种memcpy的

我还建议你重新考虑一下函数的参数，如果你把它们放在结构你有更好的控制，并使该函数更具可读性，例如

int foo(input* inbalance, output* outbalance) 

（或不管它是你正在尝试做的）

Answer 2

我的感觉是，这段代码非常脆弱。如果给出了很好的输入（我不打算让桌面检查你的东西），它可能会工作，但如果给出一些不正确的输入，它会崩溃或烧伤或给出令人误解的结果。

您是否测试了意外输入？例如：

• 假设i_balance_info为空？
• 假设i_balance_info是“”？

• 假设输入字符串中少于8个项目，这行代码将执行什么操作？

  memcpy(o_balance_sign,str[7],1); 
  

• 假设，在海峡项目[3]少于16个字符长，会有什么这行代码呢？

  memcpy(o_balance_change,str[3],16); 
  

我的方法来编写这样的代码将防止所有这些可能性。至少我会添加ASSERT（）语句，我通常会写明确的输入验证并在错误时返回错误。这里的问题在于界面似乎不允许任何可能存在错误输入的可能性。