Memory leak in a join string function

Question

I'm trying to make a function that join 2 strings (str1 & str2) into a new one (str3) seperated by a char seperator. Unfortunately I have a memory leak from this function and I don't really know why since I free str3 at the end.

Example: str_join_string("ABC","DEF",'|') ---> "ABC|DEF"

Here's the code:

char *str_join_string(const char *str1, const char *str2, char separator) {
  char *str3;
  size_t len = str_length(str1)+ str_length(str2)+1;
  size_t i = 0;
  size_t j = 0;

  str3 = (char * )calloc(len, sizeof(char));
  if(str3 == NULL){
    printf("Impossible d'allouer la mémoire");
    return NULL;
  }
  

  while(str1[i] != '\0' && str1 != NULL){
    str3[i] = str1[i];
    i++;
  }
  str3[i] = separator;
  i+=1;

  while(str2[j] != '\0' && str2 != NULL){
    str3[i+j] = str2[j];
    j++;
  }
  str3[len] = '\0';
  
  return str3;
}

I will add that I can't use any function like strcat() or anything that comes from string.h.

What Valgrind shows:

==4300== Searching for pointers to 3 not-freed blocks
==4300== Checked 131,560 bytes
==4300== 
==4300== 4 bytes in 1 blocks are definitely lost in loss record 1 of 3
==4300==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4300==    by 0x13E3B2: str_join_string (stringslib.c:238)
==4300==    by 0x13E545: str_join_array (stringslib.c:283)
==4300==    by 0x137065: JoinArrayTest_OneEmpty_Test::TestBody() (stringslib_test.cc:779)
==4300==    by 0x1652A9: HandleSehExceptionsInMethodIfSupported<testing::Test, void> (gtest.cc:2611)
==4300==    by 0x1652A9: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2647)
==4300==    by 0x15A9DE: testing::Test::Run() [clone .part.658] (gtest.cc:2686)
==4300==    by 0x15AC61: Run (gtest.cc:2677)
==4300==    by 0x15AC61: testing::TestInfo::Run() [clone .part.659] (gtest.cc:2863)
==4300==    by 0x15B350: Run (gtest.cc:2837)
==4300==    by 0x15B350: testing::TestSuite::Run() [clone .part.660] (gtest.cc:3017)
==4300==    by 0x15BAF4: Run (gtest.cc:2997)
==4300==    by 0x15BAF4: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:5709)
==4300==    by 0x165769: HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (gtest.cc:2611)
==4300==    by 0x165769: bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (gtest.cc:2647)
==4300==    by 0x15AD82: testing::UnitTest::Run() (gtest.cc:5292)
==4300==    by 0x11C08E: RUN_ALL_TESTS (gtest.h:2485)
==4300==    by 0x11C08E: main (stringslib_test.cc:799)
==4300==

I hope that you can help me because I'm really lost right now.

------EDIT------ Yes I completely forgot to add the caller which is where I free the memory:

TEST(JoinStringTest, Simple) {
  char *buf = str_join_string("ABC", "XYZ", '|');
  ASSERT_TRUE(buf != NULL);
  EXPECT_EQ(buf[0], 'A');
  EXPECT_EQ(buf[1], 'B');
  EXPECT_EQ(buf[2], 'C');
  EXPECT_EQ(buf[3], '|');
  EXPECT_EQ(buf[4], 'X');
  EXPECT_EQ(buf[5], 'Y');
  EXPECT_EQ(buf[6], 'Z');
  EXPECT_EQ(buf[7], '\0');
  free(buf);
}

When a function returns, it has finished executing. Your free(str3) line will never be called and is "dead code". str_join_string cannot free str3 anyway since it's returning it to the caller; the caller must be responsible for freeing the returned value when it no longer needs it. — jamesdlin
Indeed, I just forgot to add the caller to the post which is where I use free() I will delete thos lines of code. Sorry. — Arthur Btrd
What is str_length()? How does it differ from the standard library's strlen()? — John Bollinger
@JohnBollinger str_length() return the same result as strlen() but for this assignment it was forbidden to use strlen() I had to make one myself. — Arthur Btrd
Are you sure you've presented the test to which Valgrind attributes the leak? JoinArrayTest_OneEmpty_Test::TestBody() from Valgrind's stack trace does not appear to match up with TEST(JoinStringTest, Simple). — John Bollinger

Vlad from Moscow Vlad from Moscow · Accepted Answer · 2020-09-21T17:44:15

For starters the function invokes undefined behavior because there is not enough memory allocated for the result string.

Instead of

size_t len = str_length(str1)+ str_length(str2)+1;

you have to write

size_t len = str_length(str1)+ str_length(str2)+2;

Moreover this statement

str3[len] = '\0';

also tries to write to the memory outside the allocated array.

It seems you mean

str3[i + j] = '\0';

Though you could remove this statement because you are using the function calloc that sets the allocated memory with zeroes. On the other hand, using calloc is inefficient in the function context.

And statements after the return statement

//...
return str3;
free(str3);
str3 = NULL;

are never executed.

Pay attention to that conditions like in this for loop

while(str1[i] != '\0' && str1 != NULL){

do not make a sense. At least the operands of the logical AND operator shall be exchanged like

while( str1 != NULL && str1[i] != '\0' ){

Though in any case the condition str1 != NULL is redundant or you could check the condition before the loop in an if statement.

Here is a demonstrative program that shows how the function can be defined (without using standard string functions) and called.

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

size_t str_length( const char *s )
{
    size_t n = 0;
    
    while ( *s++ ) ++n;
    
    return n;
}

char * str_join_string( const char *s1, const char *s2, char separator ) 
{
    size_t n = str_length( s1 ) + str_length( s2 ) + sizeof( separator ) + 1;
    
    char *s3 = malloc( n );
    
    if ( s3 )
    {
        char *p = s3;
        
        for ( ; *s1; ++s1 ) *p++ = *s1;
        
        *p++ = separator;
        
        for ( ; *s2; ++s2 ) *p++ = *s2;
        
        *p = '\0';
    }
    
    
    return s3;
}

int main(void) 
{
    char *s = str_join_string( "ABC", "DEF", '|' );
    
    if ( s ) puts( s );
    
    free( s );
    
    return 0;
}

The program output is

ABC|DEF

It is the user of the function shall provide arguments not equal to NULL.

Memory leak in a join string function

3 Answers