Memory leak issue

[Labdabeta]

Hello,

I was recently looking through some of my code and found what I believe to be a memory leak. It is in a function that appends two strings and I am not sure how to resolve it. Here is the function:

const char *strapp(const char *str,const char *s)
{
    int size,strSize,sSize;
    for (strSize=0;str[strSize];++strSize);
    for (sSize=0;s[sSize];++sSize);
    size=strSize+sSize;
    char *ret=new char[size+1];
    for (size_t i=0; i<strSize; ++i)
        ret[i]=str[i];
    for (size_t i=0; i<sSize; ++i)
        ret[strSize+i]=s[i];
    ret[size]=0;
    return ret;
}

Note that this includes a new operator but would leave it up to the untrustworthy user of the function to delete the resulting pointer (which I didn't believe was possible, since it is a const pointer, but apparently it is). I am wondering if there is a way around this without using the <cstring> or <string> libraries (as they are overkill in this situation).

[Moschops]

C++11 comes with a shared_pointer template that could be used here, I think. You'd need to create the shared_pointer with a custom deleter parameter so that delete[] is called rather than delete.

You return the shared_pointer, and when it falls out of scope at the caller, the array is deleted.

[Labdabeta]

The issue is that the project that this is in was created before the C++11 standard, and thus some of the variable names have been claimed as keywords. I temporarily solved the issue like so:

const char *strapp(const char *str, const char *s)
{
    string ret="";
    ret+=str;
    ret+=s;
    return ret.c_str();
}

but that is some significant overkill right there and this function is called inside a loop, so I want it to be efficient. I know that string literals go out of scope at the end of a file, is there no way to emulate that without globals or shared/unique_pointers or wrapper classes?

EDIT: I know it must be possible somehow, since <cstring> includes the strcat() function which does exactly what I want. The issue is that this is old code and sadly used many very poor variable names (strstr being one of them, explicit being another, so no <cstdlib> and no C++11)

[Labdabeta]

It turns out that a deeper look at strcat reveals me to be an idiot. It works by absorbing a buffer and using that, so that the user can be trusted to delete it manually (or suffer the consequences). Without that I would indeed require that the user deal with cleanup or a wrapper class. Thank you for the help anyways.

[triumphost]

Don't mean to grave dig a 2 day old thread but are you sure:

const char *strapp(const char *str, const char *s)
{
    string ret="";
    ret+=str;
    ret+=s;
    return ret.c_str();
}

is valid? I ask because I'm wondering what will happen to the returned result when ret (its owner) gets destroyed. I think it might be undefined behaviour.

[Labdabeta]

It worked with my compiler and it worked on ideone (http://ideone.com/Gnxc32), but I expect it probably is undefined.

From what I understand of the situation you cannot create a const char pointer without relying on somebody else to clean it up unless it is a string literal or it uses a predefined buffer size (because you could just use char ret[BUFFER_SIZE]).

[triumphost]

You could do what microsoft does in WinAPI.

Call once to get the length required. The second call to the function will use a buffer with that length.

C functions also do this. One example is mbstowcs. This way the user allocates their own memory and knows that they must clean it up.

Example:

int strapp(const char* str_one, const char* str_two, char* result)
{
    int size_one = 0;
    int size_two = 0;

    while(str_one[size_one++]);
    while(str_two[size_two++]);  
    int total_size = size_one + size_two;

    if (!result) return total_size;

    int i = 0, j = 0;
    while(str_one[i]) result[i] = str_one[i++]; 
    while(str_two[j]) result[i++] = str_two[j++];

    result[i] = '\0';
    return total_size;
}

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

int main()
{
    int size = strapp("hello", " world", NULL);
    char* str = malloc(size + 1);

    strapp("hello", " world", str);
    printf("%s", str);

    free(str);
}

It is annoying having to calculate length of both strings twice though.. So I guess your solution really might be the best there is..

[mike_2000_17]

is valid? I ask because I'm wondering what will happen to the returned result when ret (its owner) gets destroyed. I think it might be undefined behaviour.

It is indeed undefined behavior (UB). But, remember, "everything works fine" is still one possible behavior when you have a UB situation, along with a ham sandwich popping out of your USB plug. Undefined is undefined, anything can happen. Although this code is UB, it is very likely that in most situations, it will work correctly because the deleted memory (that the returned pointer points to) is probably not yet overwritten or reclaimed by the OS immediately after you have returned from the function. But on some platforms (or sporadically), this could lead to real problems (access violation / seg-fault, heap-corruption, etc.).

This whole situation is really the reason why we don't use these kinds of primitive constructs (C-style strings) in production code. You need to use some kind of RAII class for any dynamically allocated memory like that, such as the std::string class. And if you have to use such primitive devices, you have to do as Labdabeta found out, passing in a buffer to store the output (along with a max size for that buffer) to have at least some minimal guarantee of correctness.