list[idx] = strdup(tok[1] +tok[2]) or initial malloc to size of list -C

Question

I have a code that has an invalid write in valgrind. What I tried to do was strcat the idx I malloc'd. I would like to know what size to strdup() so I can put spaces in between the number of tokens, which are the in the char *argv[].

Here is a small piece of the code

If you are interested in a larger picture of how the tokens are collected form getline you can see the question here https://stackoverflow.com/questions/27394313/getline-loop-for-stdin-has-segmentatoin-fault-after-each-loop-c

// parent will wait for child to exit
id = wait( &status );

if( id < 0 ) {
    perror( "wait" );

} else {

    int idx = 0;
    histL[count-1] =  strdup(argv[idx]);
    //printf("Token added %s size of %i is\n", argv[idx], i);
    for (idx = 1; idx < i-1; idx++) {       //-1 because no need last one
        strcat( histL[count-1], " ");
        strcat( histL[count-1], argv[idx]);
        //printf("Token added %s\n", argv[idx]);
    }
    //printf("Exit for Loop\n");
//puts( "Parent is now exiting." );

//exit( EXIT_SUCCESS );
    }

Here is the best that I can do at this point

int idx = 0;
histL[count-1] =  ( char* )malloc( sizeof( argv ) + (i*sizeof(" ")));// histLSize );    //strdup(argv[idx]);
strcpy(histL[count-1], strdup(argv[idx]));
//printf("Token added %s size of %i is\n", argv[idx], i);
for (idx = 1; idx < i-1; idx++) {       //-1 because no need last one
    histL[count-1] = strcat( histL[count-1], " ");
    histL[count-1] = strcat( histL[count-1], argv[idx]);
    //printf("Token added %s\n", argv[idx]);
}

Fixed it

int idx = 0;
histL[count-1] =  ( char* )malloc( sizeof( &argv ) + (i*sizeof(" ")));// histLSize );   //strdup(argv[idx]);
strcpy(histL[count-1], argv[idx]);
//printf("Token added %s size of %i is\n", argv[idx], i);
for (idx = 1; idx < i-1; idx++) {       //-1 because no need last one
    histL[count-1] = strcat( histL[count-1], " ");
    histL[count-1] = strcat( histL[count-1], argv[idx]);
    //printf("Token added %s\n", argv[idx]);
}

A string returned from `strdup` will have *NO* room for concatenation. The memory allocation is sized specifically to the string length of the string passed + 1 for the terminator. I.e. both of your `strcat` invokes are *guaranteed* to invoke *undefined behavior*. (ok, the second one *may* not, but only if it is a zero-length string, which would be pointless; the damage is already done from the first one regardless). — WhozCraig, Dec 11 '14 at 04:08
Alright Whoz Craig, you are right, the first error says invalid write of size `==18236== Invalid write of size 2 ==18236== at 0x401823: main (mysh.c:424) ==18236== Address 0x51fd284 is 4 bytes inside a block of size 5 alloc'd ==18236== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64` I would try the strcats like `histL[count-1] = strdup(strcat( histL[count-1], " "));` — , Dec 11 '14 at 04:10
That still don't do it. You need a buffer large enough to hold the length of *all* strings plus a terminator. Burying `strcat( histL[count-1], " ")` inside a `strdup` isn't going to change the undefined behavior being invoked. — WhozCraig, Dec 11 '14 at 04:12
I would definitely just say `histL[count-1] = strdup(argv);` and throw the whole array but I need a space between each index. It might seem like I should find number of tokens first. histL[count-1] = (char*)malloc(sizeof(argv)+ (sizeof(" "))*idx);` What do you think man? Thanks for the comments, I hope I am addressing your concern with my solution. — , Dec 11 '14 at 04:14
It is highly likely that *still* won't do it, That`malloc` is very likely incorrectly requesting the size of a pointer-to-pointer (a `char**`) plus 2 (`sizeof(" ")`), assuming `argv` is either `char* argv[]` or `char ** argv`, which are equivalent. If `argv` is `char argv[M][N]` a *true* array of arrays, it *may* work. C pointer code isn't something you just throw up and hope it sticks. You need to *know* what you're doing or catastrophe will be in the making. — WhozCraig, Dec 11 '14 at 04:20
hwo to fix code indentation when copying code that is already indented? — , Dec 11 '14 at 05:00

Remy Lebeau · Answer 1 · 2014-12-11T17:47:39.763

You are not calculating the malloc() size correctly at all. You need to use two loops, one to calculate the size, then another to copy the strings after allocating the memory:

int lastidx = i-1;

int size = strlen(argv[0]) + 1;
for (int idx = 1; idx < lastidx; ++idx) {
    size += (1 + strlen(argv[idx]));
}

histL[count-1] = (char*) malloc(size);

strcpy(histL[count-1], argv[0]);
for (int idx = 1; idx < lastidx; ++idx) {
    strcat(histL[count-1], " ");
    strcat(histL[count-1], argv[idx]);
}

Or better:

int lastidx = i-1;

int size = strlen(argv[0]) + 1;
for (int idx = 1; idx < lastidx; ++idx) {
    size += (1 + strlen(argv[idx]));
}

char *str = (char*) malloc(size);

int len = strlen(argv[0]);
memcpy(str, argv[0], len);
size = len;

for (int idx = 1; idx < lastidx; ++idx) {
    str[size++] = ' ';
    len = strlen(argv[idx]);
    memcpy(str+size, argv[idx], len);
    size += len;
}

str[size] = '\0';
histL[count-1] = str;

I don't mean to be critical in my comments. Whozcraig and you answer recieved 0 errors. Great job, I like how you included the " " which were needed — , Dec 11 '14 at 04:53
Apologies amok, Remy, I totally spaced the embedded space. glorious answer, and +1. — WhozCraig, Dec 11 '14 at 05:15

list[idx] = strdup(tok[1] +tok[2]) or initial malloc to size of list -C

1 Answers1