corruption of the heap with char array passed to function

[dospy]

i want to construct a Message class which encapsulates and handles data for sending over a network; i want to acheive something like this:

Client->Send(new CMsgPacket("Some Message", CHAT_COLOR_WHITE, CHAT_TYPE_SERVICE));
// send function deletes the data after processing it
// this is the CMsgPacket class

class CMsgPacket
{
private:
    int Msg_Len, To_Len, From_Len;
    char* m_Msg, *m_To, *m_From;
    bool new_Msg, new_To, new_From;
public:
    CHAT_TYPE ChatType;
    DWORD Color;
    DWORD SenderUID;

    char* GetMessage( );
    void SetMessage(char* value);
    __declspec(property(put = SetMessage, get = GetMessage)) char* Message;
    char* GetTo( );
    void SetTo(char* value);
    __declspec(property(put = SetTo, get = GetTo)) char* To;
    char* GetFrom( );
    void SetFrom(char* value);
    __declspec(property(put = SetFrom, get = GetFrom)) char* From;

    CMsgPacket( );
    CMsgPacket::CMsgPacket(char* _Message, CHAT_COLOR _Color, CHAT_TYPE _ChatType);
};






char* CMsgPacket::GetMessage( ) { return m_Msg; }
void CMsgPacket::SetMessage(char* value) 
{ 
    if(new_Msg)
    {
        delete[] m_Msg;
        new_Msg = false;
    }
    m_Msg = value; 
    Msg_Len = strlen(value); 
}
//
char* CMsgPacket::GetTo( ) { return m_To; }
void CMsgPacket::SetTo(char* value) 
{ 
    if(new_To)
    {
        delete[] m_To;
        new_To = false;
    }
    m_To = value; 
    To_Len = strlen(m_To); 
}
//
char* CMsgPacket::GetFrom( ) { return m_From; }
void CMsgPacket::SetFrom(char* value) 
{ 
    if(new_From)
    {
        delete[] m_From;
        new_From = false;
    }
    m_From = value; 
    From_Len = strlen(m_From); 
}
//
//
// Size Function
WORD CMsgPacket::Size( )
{
    return 32 + From_Len + To_Len + Msg_Len;
}
CMsgPacket::CMsgPacket( )
{
    m_To = m_From = m_Msg = NULL;
    To_Len = From_Len = Msg_Len = 0;
    new_From = new_To = new_Msg = false;
    Color = SenderUID = 0;
    ChatType = CHAT_TYPE_NONE;
}
CMsgPacket::CMsgPacket(char* _Message, CHAT_COLOR _Color, CHAT_TYPE _ChatType)
{
    Msg_Len = strlen(_Message);
    m_Msg = new char[Msg_Len];
    strcpy(m_Msg, _Message);

    To = "ALL";         // <--- error at this line
    From = "SYSTEM";    // <--- also i suppose here too
    Color = _Color;
    ChatType = _ChatType;
}

the problem is i get a coruption of the heap when i am trying to initialize the char array fields
anybody has any idea how to deal with this and why is it happening? should i assign the values to variables "m_To" and "m_From" as i did with "m_Msg", using strcpy and strlen?

[RonalBertogi]

First of all, I did not see any variable declared with an identifier To and From. But I suppose the main error in this class is your declaration if these variables: m_To, m_From and m_Msg. They are all pointers. And the fact that they are pointers, you should not call the delete operator on them unless you have initialized them with the new operator.

Let's look at the mutators of these variables. Since they are all in the same code pattern, we will just look particularly at the SetMessage function specifically on line 43.

On Line 43 you assigned the value of the actual parameter called value to m_Msg data of your class using the assignment operator--WITHOUT allocating an appropriate amount of memory for m_Msg to accomodate the length of the value. In some few (and rare) cases this can be ok. But the problem is if the memory pointed to in the formal parameter value is a non-static memory where it will automatically be freed when the function where the SetMessage was called from exits.

Example:

void InitializeChat(CMsgPacket * pMsgPacket)
{
    char msg[] = "A message"; // This is a temp variable that will be
                              // destroyed when the function exits.
    pMsgPacket.SetMessage(msg);
}

void main()
{
    CMsgPacket msg;
    InitializeChat(&msg);
}

pMsgPacket.SetMessage() was called with a temporary variable as its argument that will be assigned to the m_Msg data of the CMsgPacket class. When InitializeChat exits, the value assigned to the m_Msg will no longer be valid.

I hope this reply will not cause some confusion.

[dospy]

firstly, thx for the reply.
secondly, To, From and Message are proerties declared with the

 __declspec(property(put = SetFrom, get = GetFrom)) char* From;

syntax and they are used the same way as they are used in C#:

From = stuff;   // same as SetFrom(stuff);
stuff = From;   // same as stuff = GetFrom( );

now, about the

char msg[] = "A message"; // This is a temp variable that will be
                          // destroyed when goes out of scope

i read somewhere that the string constats are available through the whole runtime of the application as they are embded in the executable itself and the compiler just calls the address location, thus not going out of scope, but maybe i didn't understand corectly.
anyway, how should a correct implementation of the constructor look like, as it doesn't quite work with strcpy(From, "somestuff); either..., it gives me a memory violation when trying to write.
i was thinking of switching to std::string to get loose from all this char* crap and reimplement the class, but how would it be possible to use char* for this?, i am curious

edit:
this class was used like this

CMsgPacket *msg = new CMsgPacket( );
msg->To = "name";
msg->From = "from";
msg->Message = "some message";
msg->Color = CHAT_COLOR_WHITE;
msg->Type = CHAT_TYPE_SYSTEM;
Client->Send(msg);

and i want to reduce this to one single line as it is ugly and i have to send messages often so this would overload my source with redundant code.

[dospy]

it turns out that i had to allocate the memory first(lol i dono how i omited that) and that's why i was getting a memory coruption; and about the default values, i managed my way around it using default parameters:

CMsgPacket::CMsgPacket(CHAT_COLOR _Color, CHAT_TYPE _ChatType, char* _Message, char* _To = "ALLUSERS", char* _From = "SYSTEM");

CMsgPacket::CMsgPacket(CHAT_COLOR _Color, CHAT_TYPE _ChatType, char* _Message, char* _To, char* _From)
{
    Msg_Len = strlen(_Message);
    m_Msg = new char[Msg_Len];
    strcpy(m_Msg, _Message);

    To_Len = strlen(_To);
    m_To = new char[To_Len];
    strcpy(m_To, _To);

    From_Len = strlen(_From);
    m_From = new char[From_Len];
    strcpy(m_From, _From);

    Color = _Color;
    ChatType = _ChatType;
}

this solved the problem

[mike_2000_17]

A heap corruption problem is almost always caused by calling delete on a pointer that does not point to memory that was allocated with new. And yes, the strcpy function does not allocate memory, it only copies data from one location to another, it is your job to allocate the memory and make sure there is enough of it to perform the entire copy. The function that actually does both an allocation and a copy is called strdup (for "string duplicate"), but be aware that this function uses malloc for the allocation, and thus the memory must be freed with free (and also, strdup is not a standard function, it is a POSIX-standard function and is available on most platforms, including Windows).

Another thing to be careful about is that C functions like strcpy are in the std namespace, like all other standard C++ library entities. Some platforms, including MSVC, have them also in the global scope, but for maximum portability you should get into the habit of always writing either std::strcpy or with a using statement (like using std::strcpy; or using namespace std; at the start of the function's body).

i was thinking of switching to std::string to get loose from all this char* crap and reimplement the class, but how would it be possible to use char* for this?, i am curious

This is a bit of a problematic question, because you are shutting down the answer as part of the question. The reality is, in C++, the proper way to deal with resources (incl. dynamically allocate memory, like raw C-style strings), is by enclosing each individual use of the resource within a class that automatically manages it, we call that a RAII class (RAII: Resource Acquisition Is Initialization, or a better term is "scope-bound resource management"). And all the C++ standard classes that deal with a resource (file, memory, etc.), like std::string, use that pattern. So, the proper way, in C++, to deal with strings (i.e. char*) is to create a class specifically for the purpose of automatically managing it, but in this case, that class already exists and it is called std::string. So, you can understand that there is really nothing that I can recommend as "good coding practice" that doesn't lead you straight to using std::string instead of bare pointers like char*.

[RonalBertogi]

i read somewhere that the string constats are available through the whole runtime of the application as they are embded in the executable itself and the compiler just calls the address location, thus not going out of scope, but maybe i didn't understand corectly.

Yes, that is, if you are certain that you will be supplying constant strings to those functions.

One more thing...

Msg_Len = strlen(_Message);
m_Msg = new char[Msg_Len];
strcpy(m_Msg, _Message);
To_Len = strlen(_To);
m_To = new char[To_Len];
strcpy(m_To, _To);
From_Len = strlen(_From);
m_From = new char[From_Len];
strcpy(m_From, _From);

...do you think that solves your problem of heap corruption? My answer is a NO. Why? Because your memory allocation was based on the return value of strlen which do not include the EOS marker. Value must be strlen() + 1.