Free allocated string after strcat

Question

According to the Allocate string (char*) in another function I have now code like this:

 void someFunction(char** string, char** argv) {
    *string = (char*)malloc(strlen(argv[2]) + 1);
    strcpy(*string, argv[2]);
    strcat(*string, ".txt"); //works fine without this
}

int main(int argc, char** argv) {
    char *string; 
    char *string2;

    someFunction(&string,argv);
    printf("%s",string);
    free(string); // freezes here due the strcat
}

After adding strcat to the code from the linked question, it freezes when I try to free my string.

That's probably because you allocated N+1 bytes and then stored N+5 bytes in it (where N is the length of the argument) — user253751, Apr 05 '16 at 22:37
before writing lines like `strcat(*string, ".txt");`, think about where in memory you are writing to, and whether you are guaranteed there is enough space allocated to write in. — M.M, Apr 05 '16 at 22:38
Side-note: since this is C, you should not cast the return value from `malloc`. — paddy, Apr 05 '16 at 22:43

score 3 · Accepted Answer · answered Apr 05 '16 at 22:38

3

You didn't allocate enough space for the resulting concatenated string; when you write past the end you invoke undefined behavior (in this case, likely heap corruption). Change the malloc to allocate strlen(argv[2]) + strlen(".txt") + 1 so you have a large enough buffer to hold the whole string.

answered Apr 05 '16 at 22:38

ShadowRanger

143,180
12
188
271

1

Note: `strlen(argv[2]) + sizeof(".txt")` also works, and would be faster, but it's a little more obscure, and people often make mistakes with `sizeof`, so I left the mention of it for a comment. – ShadowRanger Apr 05 '16 at 22:40

score 2 · Answer 2 · edited May 23 '17 at 10:28

Function someFunction has a bug. It does not allocate enough memory to append ".txt".

It could look the following way

 void someFunction(char** string, char** argv) {
    *string = (char*)malloc(strlen(argv[2]) + 1 + 4 );
    strcpy(*string, argv[2]);
    strcat(*string, ".txt"); //works fine without this
}

However in any case the design of the function is bad. It uses magic number 2 in the expression argv[2] . If the function deals with argv[2] then there is no sense to pass as an argument the whole array pf pointers argv. You should just pass argv[2]. Moreover it uses parameter string by reference but does not free the memory that the parameter can point to. So its interface is confusing.

You can use the function shown in my answer to the question Safe way to concat two strings in C

The function could be called like

char *string = getConcatString( argv[2], ".txt" );

if ( string ) puts( string );

free( string );

Free allocated string after strcat

2 Answers2