"Stack smashing detected" -- Cannot functionalize a chunk of code

[Caleb9849]

Hi all. I'm working on a pretty large project. Everything's going well except that I'm having a very strange problem trying to put a chunk of code into a separate function from main. I am communicating with a CrystalFontz-brand LCD screen + keypad unit which connects to the computer via USB. It communicates in a pretty simple packet/handshaking protocol, and I have defined a struct which generally describes a single packet of data to or from the unit:

struct crystal_packet {
     unsigned char type;
     unsigned char length;
     unsigned char data[30];
}

Now, there are many points in time at which I need to wait for a keypress from the user. Getting input from the device is a several-step process, including using a CRC algorithm to verify data validity. So obviously I want to functionalize the process of getting a key from the user, into something called, say, crystal_keystroke(). Here is the process:

crystal_packet response;
response.type = 0;
while (response.type != 128) {
     if (!crystal_get(response)) response.type = 0;
}
int keypress = response.data[0];

Most of this snippet isn't relevant to my problem -- it's just the logistics of communicating with the device -- suffice it to say that at the end of this snippet, a key on the CrystalFontz has been pressed and "keypress" contains an indicator telling me which one.

Now, this code works without a hitch *if* it's sitting right in main(). The problem is that if I try to put this *exact* same code into a separate function called crystal_keystroke(), change the last line to return response.data[0] as the result, and call "int keypress = crystal_keystroke()" from main, the program craps out with a backtrace and the complaint "Stack smashing detected." This seems to occur on the "return" line of crystal_keystroke(). I've read a little bit about what this means but I don't quite understand it, or why it's happening here. Apparently it has to do with addressing memory illegally (similar to a segfault, but somehow different), but I don't see any way I'm doing that.

Oddly (in my perception), I discovered that if I instantiate the crystal_packet struct on main's stack instead of on crystal_keystroke's stack, and pass it by reference to crystal_keystroke, everything works dandily. I.e., if I write the function like this:

int crystal_keystroke (crystal_packet & response) {
     response.type = 0;
     while (response.type != 128) {
          if (!crystal_get(response)) response.type = 0;
     }
     return response.data[0];
}

And call it like this, from main():

crystal_packet response;
int keystroke = crystal_packet(response);

Then it works without a hitch. The only thing I am trying to do differently from that is to instantiate the crystal_packet struct on crystal_keystroke's stack instead of main's stack, so that I don't have to keep a struct in memory on main's stack that I'm not using most of the time (and make main's code a bit bulkier). Why shouldn't I be able to do it that way? An additional bit of intrigue: I already have a *lot* of other functions, separate from main, that I'm using the same way as I'm trying to use crystal_keystroke(), that I've concocted for interfacing with the device in various ways. *They* instantiate crystal_packets on *their* stacks without a complaint...and I can run them as many times as I want! But a single call to crystal_keystroke() and everything explodes. Why can I not instantiate one on the stack of this function in particular?

If anyone can provide any help or insight it would be much appreciated. TIA!

[ne555]

Maybe you are accessing the array out of bounds
¿Can you show crystal_get(), please?

[cire]

I can't see anything wrong with this particular code that would cause any problem, although you didn't show the code that supposedly caused it. Assuming it was the following:

int crystal_keystroke () {
     crystal_packet response ;
     response.type = 0;
     while (response.type != 128) {
          if (!crystal_get(response)) response.type = 0;
     }
     return response.data[0];
}

it shouldn't be a problem. My first thought is that you have a problem elsewhere that only surfaces here when you modify the stack. That suggests to me that there is a previous function call that returned a pointer or reference to memory allocated on the stack which your code has stored.

[Caleb9849]

I can't see anything wrong with this particular code that would cause any problem, although you didn't show the code that supposedly caused it. Assuming it was the following:

int crystal_keystroke () {
     crystal_packet response ;
     response.type = 0;
     while (response.type != 128) {
          if (!crystal_get(response)) response.type = 0;
     }
     return response.data[0];
}

Yes, this is exactly the problematic code. Let me point out again that it doesn't seem to happen until the return statement -- if I put a "printf("Made it here\n");" immediately before the return instruction, that printf is carried out. But it doesn't seem to make it back from the function to continue with main().

Maybe you are accessing the array out of bounds
¿Can you show crystal_get(), please?

Sure, crystal_get() will follow. It's passed a crystal_packet by reference in which to store any complete packet received from the device, and it also gives back, as the function return value, a boolean indicating whether or not a full and good packet came in. I'd like to point out though that one of my other functions which instantiates a crystal_packet also uses crystal_get(), and I've tested it fairly vigorously since it is functioning as the heart and soul of input from the device. Of course, I am not so arrogant as to think therefore that nothing could *possibly* be wrong with it. ^_^

// Get incoming data
bool crystal_get (crystal_packet & result) {
	// Read as much data as possible from the device into the input buffer
	int bytes_gotten_this_time = read(pad_handle, pad_input_buffer + bytes_already_in_buffer, BUFFER_SIZE - bytes_already_in_buffer);
	// Did we get anything?
	if (bytes_gotten_this_time <= 0) return false;
	// Yep!  Let's see what it is
	bytes_already_in_buffer += bytes_gotten_this_time;
	// Need at least 4 bytes for a good complete packet
	if (bytes_already_in_buffer < 4) return false;
	// See how much data we should be getting
	int supposed_data_length = pad_input_buffer[1];
	// If the data length is too long, then something is screwed up
	if (supposed_data_length > 18) {
		// Throw away a byte, scooting the buffer over, and recurse on the rest
		memcpy(pad_input_buffer, pad_input_buffer + 1, bytes_already_in_buffer - 1);
		bytes_already_in_buffer--;
		return crystal_get(result);
	}
	// Need the data itself, the type (1 byte), the data length (1 byte), and the CRC checksum (2 bytes), for a total of 4 additional bytes
	if (bytes_already_in_buffer < supposed_data_length + 4) return false;
	// Check the CRC
	unsigned short correct_CRC = compute_crc(pad_input_buffer, supposed_data_length + 2, 0xFFFF);
	unsigned short * supposed_CRC = (unsigned short *)(pad_input_buffer + supposed_data_length + 2);
	if (correct_CRC == *supposed_CRC) {
		// We have a finished good packet.  Put it into the crystal_packet, and send back "true" to indicate
		memcpy(&(result.type), pad_input_buffer, 1);
		memcpy(&(result.length), pad_input_buffer + 1, 1);
		memcpy(&(result.data), pad_input_buffer + 2, result.length);
		memcpy(&(result.data) + result.length, pad_input_buffer + 2 + result.length, 2);
		// But we must take care.  What if we got some of the next packet already, too?
		if (bytes_already_in_buffer > supposed_data_length + 4) {
			// Move the remainder of the data to the beginning of the buffer
			memcpy(pad_input_buffer, pad_input_buffer + 2 + result.length + 2, bytes_already_in_buffer - (result.length + 4));
		}
		bytes_already_in_buffer -= (result.length + 4);
		// Let the calling code know that a good packet came in
		return true;
	}
	else {
		// The CRC is bad (we've got some junk data).  Throw away a byte and recurse on the rest
		memcpy(pad_input_buffer, pad_input_buffer + 1, bytes_already_in_buffer - 1);
		bytes_already_in_buffer--;
		return crystal_get(result);
	}
}

Thanks again for the willingness to help. :)

[Caleb9849]

Update: It's been pointed out to me that memcpy is unsafe on potentially-overlapping sections of memory, and that memmove accomplishes the same goal safely. I was previously unaware of that. The memcpy's have been changed to memmove's, and the error still occurs. I don't *think* (but again, I could be wrong) that the issue is with my crystal_get() function. I've experimented some more and found that another way to make the crystal_keystroke() function work is simply to heap-allocate the crystal_packet, like so:

int crystal_keystroke () {
	crystal_packet * response = new crystal_packet;
	response->type = 0;
	while (response->type != 128) {
		if (!crystal_get(*response)) response->type = 0;
	}
	int result = response->data[0];
	delete(response);
	return result;
}

But I shouldn't have to do this; I know that I need one and only one crystal_packet, so why can't I just create on the stack? In a nutshell, this ^ works fine, but the following causes "stack smashing detected" :

int crystal_keystroke () {
	crystal_packet response;
	response.type = 0;
	while (response.type != 128) {
		if (!crystal_get(*response)) response.type = 0;
	}
	int result = response.data[0];
	return result;
}

Why? o_O

[JLBorges]

This has all the symptoms of a canary being overwritten. http://en.wikipedia.org/wiki/Buffer_overflow_protection#Canaries

In

int crystal_keystroke () {
     crystal_packet response ;
     response.type = 0;
     while (response.type != 128) {
          if (!crystal_get(response)) response.type = 0;
     }
     return response.data[0];
}

the only local objects on the stack-frame are the crystal_packet and a canary; any buffer overflow will show up because the canary which is at an immediately higher address to the crystal_packet would get over-written. main() has a much larger stack frame, the canary may not immediately follow the crystal_packet object and an overflow by some number of bytes may go undetected. If the crystal_packet is allocated dynamically, the overflow takes place somewhere on the free store; again the canary is not tampered with and the buffer overflow goes undetected.

Make the following modification to crystal_get :

bool crystal_get (crystal_packet & result) {  
// ...
     if (correct_CRC == *supposed_CRC) {
		// We have a finished good packet.  Put it into the crystal_packet, and send back "true" to indicate
		memcpy(&(result.type), pad_input_buffer, 1);
		memcpy(&(result.length), pad_input_buffer + 1, 1);

               assert( result.length <= ( sizeof(result.data) - 4 ) ) ; // *** just being extra defensive

                // *** This code is broken ****
		//memcpy(&(result.data), pad_input_buffer + 2, result.length);
                // *** This code is broken ****
		//memcpy(&(result.data) + result.length, pad_input_buffer + 2 + result.length, 2);

		memcpy( result.data, pad_input_buffer + 2, result.length);
		memcpy( result.data + result.length, pad_input_buffer + 2 + result.length, 2);
// ... 

Type of &(result.data) is unsigned char (*pointer_to_array)[30] ; &(result.data) + result.length would give an address that is equal to result.data + result.length * sizeof(result.data) or result.data + result.length * 30. A buffer overflow is the natural consequence.

Run this tiny program and the precise problem with the original code would become apparent:

#include <iostream>

int main()
{
    struct crystal_packet {
         unsigned char type;
         unsigned char length;
         unsigned char data[30];
    } ;

    crystal_packet result ;
    std::cout << (void*)result.data << ' ' <<  &result.data << '\n'
              << (void*)(result.data+1) << ' ' << &result.data+1 << '\n';
}

[Caleb9849]

                // *** This code is broken ****
		//memcpy(&(result.data), pad_input_buffer + 2, result.length);
                // *** This code is broken ****
		//memcpy(&(result.data) + result.length, pad_input_buffer + 2 + result.length, 2);

		memcpy( result.data, pad_input_buffer + 2, result.length);
		memcpy( result.data + result.length, pad_input_buffer + 2 + result.length, 2);

You're right! I was taking the address of result.data, when result.data itself is what I wanted (it being the pointer to the start of the array). Apparently that was my whole problem. Thanks very much. I feel pretty stupid now. 9_9 What I don't understand now is how I was getting away with everything else working properly, with that coding error in place....

[JLBorges]

> What I don't understand now is how I was getting away with everything else working properly

You were not getting away with anything - it wasn't working properly. You were just plain unlucky that your testing didn't catch the error earlier.

You should probably write a simple simulator for the device (for example, one that reads and presents data from a test-file) and test your code using that.

You should certainly prefer type-safe C++ features over equivalent C features - then the C++ compiler can detect programming errors that would have gone undetected in C.

int main()
{
    struct crystal_packet {
         unsigned char type;
         unsigned char length;
         unsigned char data[30];
    } ;
    char buffer[] = "vbjvbbjbjbkjbb" ;
    crystal_packet result ;

    std::memcpy( &result.data+1, buffer, 5 ) ; // this compiles cleanly

    std::uninitialized_copy( buffer, buffer+5, &result.data+1 ) ; // this won't
    // *** error: incompatible types in assignment of 'char' to 'unsigned char [30]'
}