converting a string to a char *. Heap error

Question

I'm not a c++ expert so I was wondering if someone could explain to me what's going wrong with this code. When it gets to delete[] str I get a an error

HEAP CORRUPTION DETECTED. CRT detected that the application wrote to memory after end of heap buffer.

This seems to be telling me that my buffer isn't large enough, but I can't see why.

char* foo() 
{
    std::string s = "01";
    char* buffer = new char[s.size()+1];
    strncpy_s(buffer, sizeof(buffer), s.c_str(), s.size());
    buffer[s.size()] = '\0';
    return buffer;
}

int main()
{
    char* str = foo()
    for (int i = 0; i < strlen(str); ++i) 
    {
        std::cout << str[i];
    }
    delete[] str;
    std::getchar();

    return 0;
}

Possible duplicate: https://stackoverflow.com/q/37538/1896169 — Justin, Sep 23 '17 at 02:46
Why would you write code like this in the first place? This is clearly C++ code, not C code, so why wouldn't you just return a `std::string` instead of a `char*`? — Remy Lebeau, Sep 23 '17 at 02:58
It's just a learning experience for me on how to relate strings and char *, not production code — Adam Sturge, Sep 23 '17 at 14:47

Adam Sturge · Answer 1 · 2017-09-23T02:48:36.970

1

Okay I seem to have fixed it. sizeof(buffer) just returns 4 bytes (the size of a pointer I'm guessing). Changing it to the number of characters in the buffer seems to have worked.

strncpy_s(buffer, s.size() + 1, s.c_str(), s.size());

edited Sep 23 '17 at 02:48

answered Sep 23 '17 at 02:38

Adam Sturge

37
8

Once you try a non-ASCII string (like `wchar_t`) that code will fail once again since the elements of a wide string are not one byte each. For **portability** one should multiply the number of elements by the size of an individual element: `nElements * sizeof(element_type)`. You incurr no performance penalty as `sizeof()` is a compile-time computation – YePhIcK Sep 23 '17 at 02:48
You could just write `strcpy(buffer, s.c_str());` instead of all that gunk – M.M Sep 23 '17 at 03:05
The use of `strncpy_s` suggests an MSVC++ environment in which "just" a `strcpy` generates a security warning. I presume the OP was trying to eliminate the warning as well – YePhIcK Sep 23 '17 at 07:47
@YePhIcK disabling such warnings would be the best way to eliminate them. This example demonstrates that the warning doesn't promote security, as OP turned correct code into broken code as a result of the warning. – M.M Sep 24 '17 at 01:24

score 1 · Accepted Answer · answered Sep 23 '17 at 02:43

1

sizeof(buffer) would only return the size of the pointer and not of the buffer it points to. The correct size of the buffer is:

const auto bufSize = sizeof( *buffer ) * (s.size() + 1);

or, a bit simpler:

const auto bufSize = sizeof( char ) * (s.size() + 1);

With that the correct string copy looks like that:

strncpy_s(buffer, bufSize, s.c_str(), s.size());

answered Sep 23 '17 at 02:43

YePhIcK

5,816
2
27
52

`sizeof(char)` is always 1 – M.M Sep 23 '17 at 03:04
You are correct, `sizeof(char)==1` by definition. I was just demonstrating a general approach – YePhIcK Sep 23 '17 at 07:45

converting a string to a char *. Heap error

2 Answers2