Memory leak on deallocating char * set by strcpy?

Question

I have a memory leak detector tool which tells me below code is leaking 100 bytes

#include <string>
#include <iostream>

void setStr(char ** strToSet)
{
    strcpy(*strToSet,  "something!");
}

void str(std::string& s)
{
    char* a = new char[100]();
    setStr(&a);
    s = a;
    delete[] a;
}

int main()
{
    std::string s1;
    str(s1);
    std::cout << s1 << "\n";

    return 0;
}

According to this point number 3 it is leaking the amount I allocated (100) minus length of "something!" (10) and I should be leaking 90 bytes.

Am I missing something here or it is safe to assume the tool is reporting wrong?

EDIT: setStr() is in a library and I cannot see the code, so I guessed it is doing that. It could be that it is allocating "something!" on the heap, what about that scenario? Would we have a 90 bytes leak or 100?

Also, you either leak *all* or an allocation, or *none* of it. You can't leak a partial allocation. — Some programmer dude, Sep 29 '17 at 10:51
@Someprogrammerdude thanks, what about if `serStr()` allocates the string `"something!"` on heap? — Griffin, Sep 29 '17 at 11:03
@patatahooligan that is not the point of my question. why isn't today sunny? :) — Griffin, Sep 29 '17 at 11:04
@Griffin passing a non-cost pointer or reference to a pointer that must be `deleted[]` is a great way to leak memory. It goes directly against the point you linked to. Just because it happened to not be your source of memory leak in this code sample does not mean it is irrelevant. — patatahooligan, Sep 29 '17 at 11:08
@patatahooligan agreed on that point, but as question is mentioned in the edit part, it is a library interface and out of my control. — Griffin, Sep 29 '17 at 11:14
@Griffin then the library is requesting a pointer to pointer *because* it wants to change what it points to. I'm guessing you shouldn't use `new` at all and pass a null pointer. `setStr` will `malloc` or `new` (only the former is available if it is pure C) and return the pointer through its parameter. You need to `free` or `delete[]` accordingly of course. — patatahooligan, Sep 29 '17 at 11:17
There is no memory leak in the code you posted . It would be better to edit the question to accurately reflect the situation you are experiencing, instead of making up code. As it stands this question has no value — M.M, Sep 29 '17 at 12:51

user7860670 · Accepted Answer · 2017-09-29T19:46:48.367

3

This code does not leak and is not the same as point number 3 as you never overwrite variables storing pointer to allocated memory. The potential problems with this code are that it is vulnerable to buffer overflow as if setStr prints more than 99 symbols and it is not exception-safe as if s = a; throws then delete[] a; won't be called and memory would leak.

Updated: If setStr allocates new string and overwrites initial pointer value then the pointer to the 100 byte buffer that you've allocated is lost and those 100 bytes leak. You should initialize a with nullptr prior to passing it to setStr and check that it is not null after setStr returns so assignment s = a; won't cause null pointer dereference.

edited Sep 29 '17 at 19:46

answered Sep 29 '17 at 10:51

user7860670

35,849
4
58
84

`setStr()` is in a library and I cannot see the code, so I guessed it is doing that. Would it be like the point 3 if `serStr()` allocates the string `"something!"` on heap? – Griffin Sep 29 '17 at 11:02
3

@Griffin If `setStr` is a library function, that's is very crucial information that should have been in the question body, because it changes everything. It's very likely that the `setStr` function is a C function which emulates pass-by-reference and actually allocates memory inside itself for the string. That means you *would* have a memory leak if you allocate memory yourself. You should really take a closer look at the documentation for this `setStr` function, as it should hopefully mention it. – Some programmer dude Sep 29 '17 at 11:04
@Griffin Furthermore, if it's a C function then it will allocate memory with `malloc` (or similar) instead of `new[]`. That means you can't use `delete[]` to free that memory, but instead should use [`std::free`](http://en.cppreference.com/w/cpp/memory/c/free). – Some programmer dude Sep 29 '17 at 11:06
@Someprogrammerdude I have updated the question with that information. It makes more sense now. So would assuming it is allocating and it is less than 100 bytes, how many bytes would the leak be? btw, no documentation exists on that call :( – Griffin Sep 29 '17 at 11:09
1

@Griffin The memory leak would be what *you* initially allocated. – Some programmer dude Sep 29 '17 at 11:15
@Someprogrammerdude edit to VTT's answer would be helpful to check as correct answer to help readers. – Griffin Sep 29 '17 at 11:16

score 1 · Answer 2 · answered Sep 29 '17 at 11:34

Summing up all the comments, it is clear what the problem is. The library you are using is requesting a char **. This is a common interface pattern for C functions that allocate memory and return a pointer to that memory, or that return a pointer to memory they own.

The memory you are leaking is allocated in the line char* a = new char[100]();. Because setStr is changing the value of a, you can no longer deallocate that memory.

Unfortunately, without the documentation, we cannot deduce what you are supposed to do with the pointer.

If it is from a call to new[] you need to call delete[].
If it is from a call to malloc you need to call std::free.
If it is a pointer to memory owned by the library, you should do nothing.

You need to find the documentation for this. However, if it is not available, you can try using your memory leak detection tool after removing the new statement and see if it detects a leak. I'm not sure if it is going to be reliable with memory allocated from a library function but it is worth a try.

Finally, regarding the question in your edit, if you leak memory you leak the whole amount, unless you do something that is undefined behavior, which is pointless to discuss anyway. If you new 100 chars and then write some data on them, that doesn't change the amount of memory leaked. It will still be 100 * sizeof(char)

`setStr` does not change the value of `a` (well, not in the code that OP posted anyway) — M.M, Sep 29 '17 at 12:49
I know, but this is not the code that is actually run. OP clarified this but has neglected to remove the code from his question. See their edit. — patatahooligan, Sep 29 '17 at 12:51

Memory leak on deallocating char * set by strcpy?

2 Answers2