1
\$\begingroup\$

I have the following code compiled with Microchip's MPLab and XC8 compiler and running on a PIC18F2550 which is doing something rather odd:

char output[20];
int i = 0;

char currentStatus = readShiftReg();
for (i = 0; i < 8; i++) {
    if ((currentStatus & (1 << i)) == (1 << i))
        sprintf(output, "Sensor %d is currently on \r\n", i + 1);
    else
        sprintf(output, "Sensor %d is currently off \r\n", i + 1);
    putsUSART(output);
}    

The for loop only ever iterates once and during that one iteration it correctly runs the second sprintf, but after it runs it, the value of i is something random (26154, 8294, ...). I've tried swapping i out for another variable j assigning the value of i to j at the beginning of the loop, but the same thing still happens to i.

It seems like it's something with sprintf because when I use the debugger, the value of i isn't changed until after sprint runs. One thing to note is that the value in output is correct (i.e. "Sensor 0 is currently off \r\n") which makes this even more perplexing.

This should be a very simple piece of code, but it's not working and I'm sure there's a simple explanation. Where should I be looking?

\$\endgroup\$
9
\$\begingroup\$

Your problem is this line:

char output[20];

You are allocating 20 characters for the buffer to hold your string, then calling sprintf() to fill it.

However your format string is too long, even before the decimal value is included. Remembering that the last array element will always need to be \x00 (a NUL character) you have 19 characters to use for your message.

Count the number of characters in this string:

Sensor %d is currently off \r\n

There are 29 characters (assuming a two digit number). This results in a buffer overflow, which will start to clobber other variables and may cause your program to operate in an unexpected manner. It is one common type of security issue in C.

Increase the size of your buffer so it is large enough to contain:

  • all of the fixed letters from your message.
  • the largest number the variable i will ever hold.
  • the trailing \r\n.
  • the terminating NUL byte.
\$\endgroup\$
  • \$\begingroup\$ doh... I knew it would be something simple. Thanks! \$\endgroup\$ – Matt Ruwe Sep 28 '14 at 13:42
  • 4
    \$\begingroup\$ Also, if your compiler supports it, prefer snprintf() over sprintf(). \$\endgroup\$ – Majenko Sep 28 '14 at 14:05
6
\$\begingroup\$

Your char output declaration is too small for the string you want to write in.

Try it again with char output[50]

Your count variable i is saved directly behind the last variable of the char-array.

Something like this:

Adress       Value
0x0F00      char[0]
0x0F01      char[1]
  .           .
  .           .
  .           .
0x0F19      char[19]   <-- last variable of char
0x0F20      i          <-- counter variable

Now sprintf doesn't check the length of the array it writes to. Check the length of the array before writing it to code!

\$\endgroup\$
  • \$\begingroup\$ two perfect answers, I wish I could mark both as the accepted answer. \$\endgroup\$ – Matt Ruwe Sep 28 '14 at 13:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.