CODEWITHSUNDEEP
csegmentation-fault
termite_paste
termite_paste

Reputation: 31

Modify a string repeatedly in C

I want to modify a string like this:—

MainString="",            ToUpdate="ABC" -> return# "ABC=1"
MainString="ABC=1",       ToUpdate="ABC" -> return# "ABC=2"
MainString="ABC=2",       ToUpdate="ABC" -> return# "ABC=3"
MainString="ABC=3",       ToUpdate="XYZ" -> return# "ABC=3:XYZ=1"
MainString="ABC=3:XYZ=1", ToUpdate="XYZ" -> return# "ABC=3:XYZ=2"
MainString="ABC=3:XYZ=2", ToUpdate="XYZ" -> return# "ABC=3:XYZ=3"

I have the following function:

void UpdateString(char *MainString, char ToUpdate[20])
{
    char *pData[50][2];
    char *saveptr1=NULL;
    int i=0,j=0,nIsPresentFlag=0;
    unsigned int CdrCnt=1;
    char workbuf1[200];
    char workbuf[200];


    memset(workbuf,0,200);
    memset(workbuf1,0,200);

    if(strlen(MainString)>0)
    {
        strcat(MainString,":");
    }

    strcpy(workbuf1,MainString);

    pData[i][0]=strtok_r(workbuf1,"=",&saveptr1);
    pData[i][1]=strtok_r(NULL,":",&saveptr1);

    if(pData[i][0]) {i++;pData[i][0]=NULL; pData[i][1]=NULL;}

    while((pData[i][0]=strtok_r(NULL,"=",&saveptr1)))
    {
        pData[i][1]=strtok_r(NULL,":",&saveptr1);
        i++;
    }

    for(j=0;j<i;j++)
        if(strncmp(ToUpdate,pData[j][0],strlen(pData[j][0]))==0)
        {
            CdrCnt=atoi(pData[j][1]);
            CdrCnt+=1;
            sprintf(pData[j][1],"%d",CdrCnt);
            nIsPresentFlag=1;
            break;
        }

    if(nIsPresentFlag==1)
        for(j=0;j<i;j++)
            sprintf(workbuf,"%s%s=%s:",workbuf,pData[j][0],pData[j][1]);
    else
        sprintf(workbuf,"%s%s=%d:",MainString,ToUpdate,1);

    workbuf[strlen(workbuf)-1]='\0';


    memset(MainString,0,200);
    strcpy(MainString,workbuf);

}

Curiously, this function is working, but sometimes causing a core dump with segfault.

What is wrong with this code? Any better way I can manage the above task?

==============================================================================

EDIT 1

String declaration:

char MainString[200];

The call is like:

UpdateString((char*)&MainString,"ABC");

Upvotes: 1

Views: 122

Answers (1)

Jonathan Leffler
Jonathan Leffler

Reputation: 754420

Preliminary observation

Given this test code:

static void chkit(char *s, char *u)
{
    printf("[%s] && [%s]", s, u);
    UpdateString(s, u);
    printf(" ==> [%s]\n", s);
}

int main(void)
{
    char MainString[200] = "";

    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "XYZ");
    chkit(MainString, "XYZ");
    chkit(MainString, "XYZ");
    chkit(MainString, "DEF");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "XYZ");
    chkit(MainString, "ABC");
    chkit(MainString, "GHI");
    return 0;
}

The output I get is:

[] && [ABC] ==> [ABC=1]
[ABC=1] && [ABC] ==> [ABC=2]
[ABC=2] && [ABC] ==> [ABC=3]
[ABC=3] && [XYZ] ==> [ABC=3:XYZ=1]
[ABC=3:XYZ=1] && [XYZ] ==> [ABC=3:XYZ=2]
[ABC=3:XYZ=2] && [XYZ] ==> [ABC=3:XYZ=3]
[ABC=3:XYZ=3] && [DEF] ==> [ABC=3:XYZ=3:DEF=1]
[ABC=3:XYZ=3:DEF=1] && [ABC] ==> [ABC=4:XYZ=3:DEF=1]
[ABC=4:XYZ=3:DEF=1] && [ABC] ==> [ABC=5:XYZ=3:DEF=1]
[ABC=5:XYZ=3:DEF=1] && [ABC] ==> [ABC=6:XYZ=3:DEF=1]
[ABC=6:XYZ=3:DEF=1] && [ABC] ==> [ABC=7:XYZ=3:DEF=1]
[ABC=7:XYZ=3:DEF=1] && [ABC] ==> [ABC=8:XYZ=3:DEF=1]
[ABC=8:XYZ=3:DEF=1] && [ABC] ==> [ABC=9:XYZ=3:DEF=1]
[ABC=9:XYZ=3:DEF=1] && [ABC] ==> [ABC=10:=3:DEF=1]
[ABC=10:=3:DEF=1] && [XYZ] ==> [ABC=10:=3:DEF=1:XYZ=1]
[ABC=10:=3:DEF=1:XYZ=1] && [ABC] ==> [ABC=11:3:DEF=1:XYZ=1]
[ABC=11:3:DEF=1:XYZ=1] && [GHI] ==> [ABC=11:3:DEF=1:XYZ=1:GHI=1]

There's clearly a problem when the numbers increase from 1 digit to 2 digits.

One problem

In the code:

if (nIsPresentFlag == 1)
    for (j = 0; j < i; j++)
        sprintf(workbuf, "%s%s=%s:", workbuf, pData[j][0], pData[j][1]);

you are invoking undefined behaviour by writing into workbuf and passing it as one of the parameters. That is simply dangerous. There's a moderate chance you'll get away with it, but 'get away' is the operative term — there's no guarantee that it will work.

The overwrite problem occurs when you format the new number into insufficient space.

Working code

The code below seems to work:

#include <string.h>
#include <stdio.h>
#include <stdlib.h>

static void UpdateString(char *MainString, char ToUpdate[20])
{
    char *pData[50][2];
    char *saveptr1 = NULL;
    int   i = 0;
    int   nIsPresentFlag = 0;
    char  workbuf1[200];
    char  workbuf[200];
    char  extra[16];

    if (strlen(MainString) > 0)
        strcat(MainString, ":");

    strcpy(workbuf1, MainString);

    pData[i][0] = strtok_r(workbuf1, "=", &saveptr1);
    pData[i][1] = strtok_r(NULL, ":", &saveptr1);

    if (pData[i][0])
        i++;

    while ((pData[i][0] = strtok_r(NULL, "=", &saveptr1)) != 0)
    {
        pData[i][1] = strtok_r(NULL, ":", &saveptr1);
        i++;
    }

    for (int j = 0; j < i; j++)
    {
        if (strncmp(ToUpdate, pData[j][0], strlen(pData[j][0])) == 0)
        {
            unsigned int CdrCnt = atoi(pData[j][1]);
            CdrCnt += 1;
            pData[j][1] = extra;
            sprintf(pData[j][1], "%u", CdrCnt);
            nIsPresentFlag = 1;
            break;
        }
    }

    if (nIsPresentFlag == 1)
    {
        char *dst = workbuf;
        for (int j = 0; j < i; j++)
        {
            int n = sprintf(dst, "%s=%s:", pData[j][0], pData[j][1]);
            /* Broken if sprintf() returns -1 */
            dst += n;
        }
    }
    else
        sprintf(workbuf, "%s%s=%d:", MainString, ToUpdate, 1);

    workbuf[strlen(workbuf)-1] = '\0';

    strcpy(MainString, workbuf);
}

static void chkit(char *s, char *u)
{
    printf("[%s] && [%s]", s, u);
    UpdateString(s, u);
    printf(" ==> [%s]\n", s);
}

int main(void)
{
    char MainString[200] = "";

    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "XYZ");
    chkit(MainString, "XYZ");
    chkit(MainString, "XYZ");
    chkit(MainString, "DEF");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "ABC");
    chkit(MainString, "XYZ");
    chkit(MainString, "ABC");
    chkit(MainString, "GHI");
    chkit(MainString, "PQRSTU");
    chkit(MainString, "I");
    chkit(MainString, "I");
    chkit(MainString, "I");
    chkit(MainString, "PQRSTU");

    return 0;
}

It leaves out the memset() operations; a null terminated string can be copied over arbitrary data and as long as you don't go looking beyond the null terminator, you won't have any problems. The variable extra is used to store the new number; it avoids problems when the number changes from N to N+1 digits. The function sprintf() returns the number of characters it wrote; that is used to add the data to the work buffer safely.

Example output

[] && [ABC] ==> [ABC=1]
[ABC=1] && [ABC] ==> [ABC=2]
[ABC=2] && [ABC] ==> [ABC=3]
[ABC=3] && [XYZ] ==> [ABC=3:XYZ=1]
[ABC=3:XYZ=1] && [XYZ] ==> [ABC=3:XYZ=2]
[ABC=3:XYZ=2] && [XYZ] ==> [ABC=3:XYZ=3]
[ABC=3:XYZ=3] && [DEF] ==> [ABC=3:XYZ=3:DEF=1]
[ABC=3:XYZ=3:DEF=1] && [ABC] ==> [ABC=4:XYZ=3:DEF=1]
[ABC=4:XYZ=3:DEF=1] && [ABC] ==> [ABC=5:XYZ=3:DEF=1]
[ABC=5:XYZ=3:DEF=1] && [ABC] ==> [ABC=6:XYZ=3:DEF=1]
[ABC=6:XYZ=3:DEF=1] && [ABC] ==> [ABC=7:XYZ=3:DEF=1]
[ABC=7:XYZ=3:DEF=1] && [ABC] ==> [ABC=8:XYZ=3:DEF=1]
[ABC=8:XYZ=3:DEF=1] && [ABC] ==> [ABC=9:XYZ=3:DEF=1]
[ABC=9:XYZ=3:DEF=1] && [ABC] ==> [ABC=10:XYZ=3:DEF=1]
[ABC=10:XYZ=3:DEF=1] && [XYZ] ==> [ABC=10:XYZ=4:DEF=1]
[ABC=10:XYZ=4:DEF=1] && [ABC] ==> [ABC=11:XYZ=4:DEF=1]
[ABC=11:XYZ=4:DEF=1] && [GHI] ==> [ABC=11:XYZ=4:DEF=1:GHI=1]
[ABC=11:XYZ=4:DEF=1:GHI=1] && [PQRSTU] ==> [ABC=11:XYZ=4:DEF=1:GHI=1:PQRSTU=1]
[ABC=11:XYZ=4:DEF=1:GHI=1:PQRSTU=1] && [I] ==> [ABC=11:XYZ=4:DEF=1:GHI=1:PQRSTU=1:I=1]
[ABC=11:XYZ=4:DEF=1:GHI=1:PQRSTU=1:I=1] && [I] ==> [ABC=11:XYZ=4:DEF=1:GHI=1:PQRSTU=1:I=2]
[ABC=11:XYZ=4:DEF=1:GHI=1:PQRSTU=1:I=2] && [I] ==> [ABC=11:XYZ=4:DEF=1:GHI=1:PQRSTU=1:I=3]
[ABC=11:XYZ=4:DEF=1:GHI=1:PQRSTU=1:I=3] && [PQRSTU] ==> [ABC=11:XYZ=4:DEF=1:GHI=1:PQRSTU=2:I=3]

I did some basic checking with valgrind (and added some dynamic memory allocation) and it came up with a clean bill of health.

Note the style of diagnostic printing. It shows the inputs and the output, which is helpful. And it encloses strings in a distinctive marker (here []) so that stray spaces etc can be spotted more easily.

Upvotes: 5

Related Questions