CODEWITHSUNDEEP
c++stringcrashconcatenation
user2619824
user2619824

Reputation: 478

Crash after string concatenation in C++

I have this program that reads data from a serial port. For each line, I'm trying to concatenate the current time with the line of data. For some reason, it crashes when around the second print (it seems like at the end of the brackets?). The weird part is, that if I comment the print out, it'll still crash

char * cdata;
{
    if( BINARY_ASCII == 1 ) //right now this is just set to 0, please ignore
    {
        cdata = convertBSTRToByteArray(data , numChars);
    }
    else
    {
        cdata = convertBSTRToString(data);

        //prints the original output
        cout << "before timestamp concat is: " << cdata << "\n";

        //this is supposed to concatenate each output line with the associated time
        std::stringstream ss;
        ss << currentDateTime() << "," << cdata;
        std::string s = ss.str();
        std::strcpy(cdata,s.c_str());

        cout << "after timestamp concat is: " << cdata << "\n"; //around here it crashes
}

cout << "after the thing" << "\n"; //does not even get here

I thought that the char * data would be the issue, but I've tried initializing it like 

char *cdata = 0;

and 

char *cdata = new char [100];

to no change...

That makes me think that I did something wrong in the concatenation?

Upvotes: 0

Views: 1284

Answers (3)

Tim
Tim

Reputation: 9172

I think it's important to highlight the difference between arrays and pointers, here.

char * cdata;

This creates a pointer named cdata. It's uninitialized, so it contains some useless memory address. A pointer is just a memory address, which means it takes up 32 (or 64) bits, and that's it.

char *cdata = 0;

This creates a pointer named cdata, and initializes it to all zeros, which means it points to the 0th location in memory. This is usually used to indicate that you should not follow this pointer.

char *cdata = new char [100];

This creates a block (array) of 100 characters, but gives that array no name. Then it creates a pointer named cdata and sets it to the memory address of the unnamed 100-byte block. I.e.:

cdata [ 0x3Ad783B2 ] --------\
                              \
                               \
                               |
                               V
                             [ unnamed 100-byte block ]

The reason I'm stressing this distinction is that the next line obliterates it all:

cdata = convertBSTRToString(data);

That line sets cdata to point to whatever memory address is returned by convertBSTRToString. It does not matter what value cdata had before this line -- uninitialized, null, pointing to an unnamed block of memory -- now it is pointing to the block of memory created by convertBSTRToString.

Abusing more ASCII-art:

cdata [ 0x64ADB7C8 ] --------\
                              \
                               \
                               |
                               V
                             [ unknown size, created by convertBSTRToString ]

                             // hey, look over here!  it still exists,
                             // but we just don't point to it anymore.
                             [ unnamed 100-byte block ]

Now that that's covered, here's why it matters. This line:

std::strcpy(cdata,s.c_str());

strcpy will take the data pointed to by the second parameter, and copy it, byte-by-byte, to the location pointed to by the first parameter. It does not pay attention to buffer size. It's a really stupid copy. No safety whatsoever - that's up to you to provide.

I'm not sure what you're trying to accomplish with this line anyway, because s holds the full string data you wanted to concatenate:

cout << "after timestamp concat is: " << s << "\n";

Upvotes: 4

Mike Makuch
Mike Makuch

Reputation: 1838

You would need to first allocate a buffer big enough to contain both the convertBSTRToString plus the currentDateTime then strcpy the convertBSTRToString and then strcat the currentDateTime. strcpy won't append, strcat does.

Upvotes: 0

C. K. Young
C. K. Young

Reputation: 223133

convertBSTRToString probably allocates a new buffer that's sized exactly right to hold the BSTR you passed in. That means you cannot expand its size.

In your code, you are trying to add currentDateTime()'s result into that buffer (in addition to its existing content). The data won't fit. Thus, bad things happen.

Upvotes: 1

Related Questions