Copying a string causes instability

Go To Last Post
23 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi guys

 

Can anyone see any problems with this code?  It causes the AVR I'm using (ATMEGA 238P-PU) to restart.

 

	char keybuffer[15][35];
	int keybufferpos = -1;

.
.
.
.

if (strlen(keys)>0)
{
	for (int i=9; i>0; i--) strcpy(keybuffer[i], keybuffer[i-1]);
	strcpy(keybuffer[0], keys);
	keybufferpos = -1;
}

 

In particular, the for loop line causes the issue. If I comment that out then I get no issues.

What it does is shuffle all of the previous commands entered by the user backwards in the command line buffer (keybuffer[]), with the last one being over written.

It's almost as if this if causing a memory corruption, but I can see how. 'i' counts down backwards from 9 to 1, copying the contents of keybuffer[i-1] into the current keybuffer[i].

 

[edit] BTW I increased the dimensions of the keybuffer array for test purposes. Also, (FYI) keys[] contains the current command

This topic has a solution.
Last Edited: Mon. Oct 19, 2015 - 12:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If any of your stings have a length greater than an entry width, it could cause problems.

 

Please be more specific about your AVR model. ;)  Assuming a Mega328 you have 2KB of SRAM, right?  Whether local or global, keybuffer takes 500 bytes.  Especially if local, it could cause stack problems.

 

BTW, instead of shuffling strings I'd just use a pointer or index and make it a circular buffer.

 

Is "keys" always null-terminated at a reasonable length?

 

Guard yourself and use e.g. strncpy() and ensure null termination and similar.

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
char keybuffer[15][35];

Is that 15 35-byte buffers, or 35 15-bye buffers?   I'm never sure, so I avoid constructs like that, along with partially addressing them like "keybuffer[i]"

It's trivial to "typedef char[35] command_t;  command_t keybufffer[15]", and then you're sure...

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

westfw wrote:

char keybuffer[15][35];

Is that 15 35-byte buffers, or 35 15-bye buffers?   I'm never sure, so I avoid constructs like that, along with partially addressing them like "keybuffer[i]"

It's trivial to "typedef char[35] command_t;  command_t keybufffer[15]", and then you're sure...

True.

That said, 'tain't all that hard.

Just remember that C does not have two-dimensional arrays and that declarations mimic use.

Iluvatar is the better part of Valar.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:

If any of your stings have a length greater than an entry width, it could cause problems.

 

Please be more specific about your AVR model. ;)  Assuming a Mega328 you have 2KB of SRAM, right?  Whether local or global, keybuffer takes 500 bytes.  Especially if local, it could cause stack problems.

 

BTW, instead of shuffling strings I'd just use a pointer or index and make it a circular buffer.

 

Is "keys" always null-terminated at a reasonable length?

 

Guard yourself and use e.g. strncpy() and ensure null termination and similar.

 

lol yes I'm using an ATMEGA 328P.

 

- keybuffer[] is local. Should I try making it global? I'll give this a go although I did read somewhere a while ago that it's best to make variables local where you can which is why I made it local

- keys[] has a 30 character limit, with keys being defined as being 32 in length.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

westfw wrote:

char keybuffer[15][35];

Is that 15 35-byte buffers, or 35 15-bye buffers?   I'm never sure, so I avoid constructs like that, along with partially addressing them like "keybuffer[i]"

It's trivial to "typedef char[35] command_t;  command_t keybufffer[15]", and then you're sure...

 

 

I always assume that it's 15 versions of a 35 byte array of characters whichever language that I'm using. So far that assumption has worked out ok. The highest level always comes first.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I reduced the keybuffer back down to [10][32] (as it was before) and have made it a global variable. So far, so good.: No resets.

Keeping things simple (I'm a hobbyist C coder at best, not a pro-programmer smiley), why would that array possibly cause an issue as a local variable? Is it because it gets defined on the stack and as the stack isn't big enough it then falls apart?

Last Edited: Fri. Oct 16, 2015 - 07:34 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Well, it might not in another language, but this is C

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Defining it as a global moves it out of the stack. This may reduce the severity of a buffer overrun, but it doesn't 'fix' it. An overrun when in the stack can clobber return addresses and other state. An overrun in .data or .bss might clobber other variables in .data or .bss (or the heap if you're using it), but isn't likely to clobber the stack. So while you haven't seen any resets with the move to global, I doubt you've actually fixed the underlying problem.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What's weird is that the code does work. I can use the up and down arrow keys to populate the current command with previous ones and there are no issues.

I also don't see any problems with other variables.

 

Just for info (and I know that this doesn't prove that something nasty isn't going on):

                Program Memory Usage 	:	13682 bytes     41.8 % Full
                Data Memory Usage 	:	1170 bytes      57.1 % Full

Just to show that I have enough SRAM (Data) left (I stuffed all of my constant strings into flash (Program) memory)

Last Edited: Sat. Oct 17, 2015 - 10:50 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Stack usage isn't reported in 'Data Memory Usage'.  There is a recent thread on this subject.  Search the fora for -fstack-usage.

 

banedon wrote:
- keys[] has a 30 character limit, with keys being defined as being 32 in length.
And how are you enforcing that?  You certainly aren't enforcing anything by using strcpy().  At the very least, you should change your code to use strncpy() in place of of every instance of strcpy(), as has already been suggested.

 

From http://www.nongnu.org/avr-libc/user-manual/group__avr__string.html#ga54e4f23104fa6f722f9459d2673a1eba:

char * strcpy ( char *  dest,
    const char *  src 
  )    

 

Copy a string.

 

The strcpy() function copies the string pointed to by src (including the terminating '\0' character) to the array pointed to by dest. The strings may not overlap, and the destination string dest must be large enough to receive the copy.

 

Returns

The strcpy() function returns a pointer to the destination string dest.

 

Note

If the destination string of a strcpy() is not large enough (that is, if the programmer was stupid/lazy, and failed to check the size before copying) then anything might happen. Overflowing fixed length strings is a favourite cracker technique.

 

From http://www.nongnu.org/avr-libc/user-manual/group__avr__string.html#ga81577c743915e4fb8759ef9081f10838:

char * strncpy ( char *  dest,
    const char *  src,
    size_t  len 
  )    

Copy a string.

 

The strncpy() function is similar to strcpy(), except that not more than n bytes of src are copied. Thus, if there is no null byte among the first n bytes of src, the result will not be null-terminated.

In the case where the length of src is less than that of n, the remainder of dest will be padded with nulls.

 

Returns

The strncpy() function returns a pointer to the destination string dest.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The user input is regulated by the following code:

 

	// check to ensure that the buffer isn't full. if it is then don't allow further input
	if (keyscount<30)
	{
		if (rawkey>=65 && rawkey<=90) rawkey += 32;		// convert upper case to lower case
		if (rawkey>=32 && rawkey<=126)
		{
			// as none of the special keys have been detected we need to send a copy of the key back
			// to the terminal to be displayed
			Serial0_SendByte(rawkey);
			//insert the key value (rawkey) into the buffer (keys[]) then increase the buffer length
			// counter (keyscount)
			keys[keyscount] = rawkey;
			keyscount++;
		}
	}

 

Here's the entire main loop with the above code in its context:

 

	int keybufferpos = -1;

	int keyscount = 0;
	char keys[32]= {0};
	sei();
	set_sleep_mode(SLEEP_MODE_IDLE);

	rawkey = 0;
	while(1)
	{
		sei();
		sleep_mode();
		if (rawkey==0) continue;
		cli();

		// check for special keys such as ENTER and BACKSPACE, cursor up or down
		if (rawkey==1)
		{
			if ((keybufferpos < 9) && (strlen(keybuffer[keybufferpos+1])==0)) continue;
			if (keybufferpos < 9) keybufferpos++;
			strcpy(keys, keybuffer[keybufferpos]);
			Serial0_SendANSIcmd(ansi_eraseline1);
			displaytext("\r>");
			displaytext(keys);
			keyscount = strlen(keys);
		}
		else if (rawkey==2)
		{
			if ((keybufferpos > 0) && (strlen(keybuffer[keybufferpos-1])==0)) continue;
			if (keybufferpos > 0) keybufferpos--;
			strcpy(keys, keybuffer[keybufferpos]);
			Serial0_SendANSIcmd(ansi_eraseline1);
			displaytext("\r>");
			displaytext(keys);
			keyscount = strlen(keys);
		}
		else if (rawkey==13)
		{
			displaytext("\n\r");
			keys[keyscount] = 0;
			if (strlen(keys)>0)
			{
				for (int i=9; i>0; i--) strcpy(keybuffer[i], keybuffer[i-1]);
				strcpy(keybuffer[0], keys);
				keybufferpos = -1;
			}
			cmd_Lookup(keys);
			keyscount = 0;
			Serial0_SendChar('>');
		}
		else
		{
			// check to see if key is backspace
			if (rawkey==8 || rawkey==127)
			{
				// if backspace then only allow it if buffer length (keyscount) > 0
				if (keyscount>=1)
				{
					// set the current buffer (keys[]) location contents to 0 (indicating end of string)
					// and decrease the count (keyscount) by 1
					keys[keyscount] = 0;
					keyscount--;
					// echo the backspace to the terminal
					Serial0_SendByte(8);
					Serial0_SendByte(' ');
					Serial0_SendByte(8);
				}
			}
			else
			{
				// check to ensure that the buffer isn't full. if it is then don't allow further input
				if (keyscount<30)
				{
					if (rawkey>=65 && rawkey<=90) rawkey += 32;		// convert upper case to lower case
					if (rawkey>=32 && rawkey<=126)
					{
						// as none of the special keys have been detected we need to send a copy of the key back
						// to the terminal to be displayed
						Serial0_SendByte(rawkey);
						//insert the key value (rawkey) into the buffer (keys[]) then increase the buffer length
						// counter (keyscount)
						keys[keyscount] = rawkey;
						keyscount++;
					}
				}

			}
		}
		//_delay_ms(25);
		rawkey = 0;
	}

 

Feel free to advise if/where I've gone wrong with my limiting.

 

 

Last Edited: Sat. Oct 17, 2015 - 04:19 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

And here's the USART ISR:

 

// ISR for USART incoming data. It fires when an entire character/byte (8 bits) has been received.
// USART_specialkey is used to keep track of ansi ESC sequences generated when the cursor keys are used
// USART_nospecialkey is used to stop interpretation of the cursor keys and just pass everything along including ESC sequences
ISR(USART_RX_vect)
{
	// grab the incoming character from USART UDR0 register and put it in rawkey
	// rawkey is picked up by the main while() loop in main(void)
	rawkey = UDR0; 

	// intercept special keys
	if (USART_nospecialkey==0)
	{
		if ((rawkey == 27) && (USART_specialkey == 0))	{ USART_specialkey = 1; rawkey = 0; return; }
		if ((rawkey == '[') && (USART_specialkey == 1))	{ USART_specialkey = 2; rawkey = 0; return; }
		if ((rawkey == 'A') && (USART_specialkey == 2))	{ USART_specialkey = 0; rawkey = 1; return; }
		if ((rawkey == 'B') && (USART_specialkey == 2))	{ USART_specialkey = 0; rawkey = 2; return; }

		USART_specialkey = 0;
	}
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'm not going to try to untangle that until you post all of the relevant code.  Where is rawkey defined?

 

You have three choices:  1) post your entire program and hope that someone has the patience to review it all for free, or 2) hope that someone can spot a problem in the fragment of code you have shown, or 3) whittle down your code to the smallest possible test program which still demonstrates the problem.

 

I vote for 3).

 

As far as regulating user input, the code you mention does nothing to prevent an overrun on keybuffer[][].  There seems to be no check that keybufferpos < 15 (or whatever the bound is).

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Fine by me.  I didn't want to post the whole thing as I didn't want people to have to wade through masses of code and as you seemed to indicate that I might not have done any length checking I thought I'd show that I have.

 

The bit that does the over run check is (it's at the beginning of the 1st bit of code post that's in bold):

 

				// check to ensure that the buffer isn't full. if it is then don't allow further input
				if (keyscount<30)
				{

 

Either way, I appreciate people taking the time to comment and make constructive suggestions as I'm a beginner C coder.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Assuming this is a bounds test for the array keys:

// check to ensure that the buffer isn't full. if it is then don't allow further input
				if (keyscount < sizeof(keys))
				{

This helps to get rid of 'magic numbers' which your code is littered with. Wikipedia has a good description of what 'magic numbers' are.

 

I'd also suggest you get into the habit of stepping through your code is the debugger or simulator and see what is happening.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for both suggestions.

In my defence I have done my best to comment my code to make it less mysterious, but I do appreciate that your way is better and easier to read (which I'll use in future).

Maybe another way of doing this would be to do something like:

 

MAX_KYBD_BUFFER_SIZE 30;

.
.
.
.

// check to ensure that the buffer isn't full. if it is then don't allow further input
				if (keyscount < MAX_KYBD_BUFFER_SIZE)
				{

 

Last Edited: Sun. Oct 18, 2015 - 08:55 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'm learning lots about C - especially with regard to AVRs - from many sources. One of which are these forums. I appreciate the help I get and try to absorb what people say.

If what I give you guys in the way of code or methodology is less than helpful then please let me know (as above).

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I don't think the compiler would accept the above code.

I think you meant:
#define MAX_KYBD_BUFFER_SIZE 30

Where possible, you want to use the language features to avoid simple human mistakes and have it work for you. The compiler is faster and more reliable than a human.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
I don't think the compiler would accept the above code. I think you meant: #define MAX_KYBD_BUFFER_SIZE 30 Where possible, you want to use the language features to avoid simple human mistakes and have it work for you. The compiler is faster and more reliable than a human.
Sorry, yes that is what I meant.

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

banedon wrote:
The bit that does the over run check is (it's at the beginning of the 1st bit of code post that's in bold):

				// check to ensure that the buffer isn't full. if it is then don't allow further input
				if (keyscount<30)
				{

The variable keyscount is never used to index keybuffer[][], only local variables like 'i', but mostly keybufferpos.  Where is the bit of code which constrains keybufferpos?  You have not shown it.

 

I'm afraid the code posted thus far is a bit spaghetti-like.  No insult intended.  However, it should have become apparent to you by now what a challenge you have set for yourself.  You started out experiencing random resets.  That reeks of a class of simple yet insidious bugs (out-of-bound buffer access, uninitialised local, 'rogue' pointer, etc.) which can be very difficult to track down.  The task gets that much more difficult with hard-coded constants, lots of special cases, and generally unstructured code.

 

The fact that your problems disappeared when you moved keybuffer[][] to be a global instead of a local IMO re-enforces this diagnosis.  You have by no means fixed your problem, you have only hidden it away.  The nature of this class of bugs in C also means that there is no guarantee that the actual underlying bug has anything whatsoever to do with keybuffer[][].  We only know that moving the location of it in SRAM from the stack to .bss has changed the behaviour of the program.  Generally, this should absolutely not be the case.

 

There are exceptions.  It could be something quite simple.  For instance, automatics are uninitialised, unless you specifically give them an initial value.  That means that whatever value is in that part of the stack (or GP register space) at the moment the variable is created, that's the value the variable will take on.  The same is true for arrays of char.  Since your code uses those to hold C 'strings' i.e. NULL-terminated char arrays, it may simply be that some part of your code is examining an array with the assumption that, if it hasn't been written to yet, it is empty.  This will not necessarily be true for an automatic.  In fact, it is almost certainly >>not<< true.  Globals, however, are initialised to zero unless specifically initialised to some other value.  This might 'save' your program, but it is not a 'fix'.  It is not IMO a good idea to rely on this difference.  At an absolute minimum, you should very clearly document every part of your code which relies on the initial zero state of keybuffer[][].  Preferable would be to explicitly initialise it yourself at an appropriate place.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Sun. Oct 18, 2015 - 08:56 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ahh right. I didn't realise that stack based variables don't initialise at 0. keybuffer[][], unlike most of my variables, is not defined with an explicit starting value when delcared.

When ENTER is pressed by the user, my code attempts to shuffle the contents of keybuffer[][] and insert the latest command/user input. It does the shuffling using strlen() and of course the keybuffer[][] variable might well be too long 'cause the contents are random...

Also, I've noticed that if the AVR survived a command being entered then it would be until power off.

 

Thank you very much indeed. I'll ensure that all my char/strings are initialised with 0 in future.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

IIRC "type_t fred = { 0 };" is sufficient to initialize an entire array of struct to zeros.

The zero might not be necessary.

Iluvatar is the better part of Valar.