1

I am trying to a create a function that keeps on appending a string to a char variable. However, some times it works and other times it doesn't. I am wondering where the bug is?

char *final_output = NULL;
void add_string(const char *);

int main(void) {
    add_string("Hello world\n");
    add_string("This is my new function!\n");

    /* Let's print */
    while (final_output && *final_output) {
        printf("%c", *final_output);
        *final_output++;
    }
}

void add_string(const char *text) {
    if (final_output == NULL) {
        final_output = malloc(strlen(text) + 1);
    }
    else {
        final_output = (char *) realloc(final_output, strlen(final_output) + strlen(text) + 2);
    }

    strncat(final_output, text, strlen(text));
}
AntikM
  • 636
  • 4
  • 13
  • 28
  • 2
    It's redundant to use `strncat()` when the length is `strlen(text)`, since that's exactly what `strcat()` does without having to read the string twice. – Barmar Nov 02 '14 at 08:31
  • 2
    The +2 is equally redundant. you only need +1 for the sole terminator the resulting string will house. And I hope you realize you just lost your only pointer to the string as soon as you walk to the terminator in your output loop. (iow, how are you planning on freeing that thing?). Finally, stop casting allocation functions in C. [No good can come of it](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). – WhozCraig Nov 02 '14 at 08:32
  • What does it do when it doesn't work? I wasn't able to get it to fail. – Barmar Nov 02 '14 at 08:34
  • Your `while` loop is is equivalent to `printf("%s", final_output)`, and that doesn't have the problem that @WhozCraig points out. – Barmar Nov 02 '14 at 08:37
  • As long as were on the subject of questionable code, `*final_output++;` results in a harmless (losing the pointer you need to free not withstanding), but none-the-less meaningless dereference. The pointer is incremented, but the dereference is worthless. – WhozCraig Nov 02 '14 at 08:39
  • Thanks for all your help guys. I made the necessary modifications as suggested by you guys. I didn't realise losing the head of a string meant I was causing memory leaks as well. In that case, what is the best way to loop through a pointer and still retain the memory address for freeing? – AntikM Nov 02 '14 at 08:45
  • [Something like this springs to mind](http://ideone.com/6eAk0S). Note: not checked beyond making sure it compiles). – WhozCraig Nov 02 '14 at 08:46
  • this line: is in error. It looses the pointer to the allocated memory. Also, the allocated memory is not being free'd resulting in a resource loss. – user3629249 Nov 02 '14 at 10:57
  • the code block, beginning with: while (final_output && *final_output) { does not work correctly. suggest replacing the code block with a simple: printf("%s\n", final_output ); – user3629249 Nov 02 '14 at 11:00
  • the function realloc() can be used with a pointer to null, so there is no need to have the if/else sequence, just the line with the realloc statement. – user3629249 Nov 02 '14 at 11:02
  • Using realloc on its own produces segmentation fault if the pointer is null. Don't know why... – AntikM Nov 04 '14 at 19:49

1 Answers1

2

The problem is in function add_string. You do not append the allocated or copied array with the terminating zero after statements

final_output = malloc(strlen(text) + 1);

and

strncat(final_output, text, strlen(text));

Rewrite the function the following way

void add_string( const char *s ) 
{
    if ( final_output == NULL ) 
    {
        final_output = malloc( strlen( s ) + 1 );
        final_output[0] = '\0';
    }
    else 
    {
        final_output = realloc( final_output, strlen( final_output ) + strlen( s ) + 1 );
    }

    strcat( final_output, s );
}
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335