printf bug - beware!

[WizardOfAz]

Well, perhaps not exactly a bug, but at minimum, a land mine.

If you code
prinft("some long string of more than 80 bytes.....\n");
you'll likely begin to have errors elsewhere in your program that are hard to explain or find. This is because the "long string" is copied from program memory (where the compiler puts constant strings) to a ram buffer of 80 bytes, but no length checking is done on the source string. If it exceeds the size of the targer buffer, it is copied in its entirety anyway, trampling other variables in the process.

I modified IFI's printf_lib.c to check for this problem and print an error rather than trampling memory. Anyone who wants the source, I'll be happy to share it.

If you don't want my fix and don't want to do your own, at least be warned that long print strings can lead to really hard to find bugs.....

Bill

[Jay Lundy]

Nice catch. The same problem occurs if you do this:

char tempBuf[] = "Some string >=40 characters...";
printf("%s", tempBuf);

The error is another strcpypgm2ram copied into a 40 char buffer without checking the length of the string in program memory.

printf_lib.c line 203 (unmodified version)

Basically anywhere in the printf libraries where strcpypgm2ram is used (which is quite often) there is a possibility for corruption.

[deltacoder1020]

tsk tsk IFI... this is something you think they would put into the docs... (and if I missed it, oh well, i was speedreading :S)

[Mark McLeod]

Originally Posted by deltacoder1020

tsk tsk IFI... this is something you think they would put into the docs... (and if I missed it, oh well, i was speedreading :S)

It all depended on where you looked.

printf_lib.c header:

* 3. The %s directive only supports a string of 40 bytes and the format string
*	 may not exceed 80 bytes.

The first time we tried to printf a "long" convinced us to toss printf_lib and write our own. Ours took up 40 bytes and worked better, printf_lib took up 4000 bytes. I know many other teams did the same, or rewrote the original printf_lib.

[10intheCrunch]

This isn't a bug either, but causes us and team 60 some amount of frustration over the weekend...

If any printf's are called before the Initialize_Serial_Coms() function is called in User Initialization, the RC will flip into a bizarre state: the disable light goes on and the program light flashes red and green. We could find no documentation on what the signal meant, and spent about 3 hours rebuilding our code before Jay noticed the errant printf at the top of User Initialization.

The problem seems pretty obvious in retrospect, but because of the obscurity of the RC feedback we were getting, I'd just like to warn other teams...don't make the same mistake =).

[Astronouth7303]

Thank you, not even IFI could tell us what alternating lights ment.

[Greg Ross]

Bill and Mark, if you could share your code, it would be appreciated.

[WizardOfAz]

Originally Posted by gwross

Bill and Mark, if you could share your code, it would be appreciated.

OK, happy to do so.

The changes are in three files.

In ifi_utilities.h I added
#define printfBufrLength 80

In ifi_utilities.c I changed the line
char ifi_printfBufr[80];
to
char ifi_printfBufr[printfBufrLength];

Neither of these were really necessary, I could have just coded "80" instead of "printBufrLength" in the changes in printf_lib.c.

This is what is added to printf_lib.c:
the function strlenrom(const rom char * string) counts the length of the string.
strlenrom() is called in several places to compare the length of the source and target.

Some will suggest that I could have used strncpypgm2ram which would limit the amount of the copy with less work. I did it as I did so as to get a clear error on the screen when the function is used improperly, rather than just omit data from the print. I suppose either way is arguable.

Here's the new printf_lib.c code:

/*******************************************************************************
* FILE NAME: printf_lib.c
* 
* 2004/03/06 (bill bennett) added length check on copy to ifi_printfBufr and to tmpBufr
*
* DESCRIPTION:
*  This file contains generic routines that work like the standard C I/O library.
*  These unsupported routines have been modified to work with the Microchip 
*  C compiler.  The printf routine is limited as follows:
*
*  1.  Only the %s, %lx, %d, %x, %u, %X directives are parsed.  An additional %b 
*      directive is parsed for those who like to view data in binary (base 2).
*
*      Examples:   
*
*            rom const char *StrPtr = "Hello world!";
*            int x = 15;
*            int y = 0x50;
*            long z = 0xdeadface;
*
*            printf("%s\n",StrPtr);                         will display 'Hello world!'
*                                  
*            printf("X = %d, Y = %x, Z = %lx\n",x,y,z);     will display 'X = 15, Y = 0x50, Z = deadface'
*                                                      
*            printf("X = %b (base 2)\n",x);                 will display 'X = 1111 (base 2)'
*                   
*            printf("X = %16b\n",x);                        will display 'X = 0000000000001111'
*                           
*            printf("X = %04x\n",x);                        will display 'X = 000f'
*                           
*  2.  All bytes (8 bits) or bit references (rc_dig_in01) must be type-cast to a  
*      16 bit word.  The %c directive is unsupported.
*
*      Examples:   
*
*            unsigned char byte1 = 0xfa;
*            char byte2 = 25;
*
*            printf("1st byte = %x, 2nd byte = %d\n",(int)byte1,(int)byte2);
*                                                           will display '1st byte = fa, 2nd byte = 25
*            if (rc_dig_in01)
*              printf("On %d\n",(int)rc_dig_in01);          will display 'On 1'
*            else
*              printf("Off %d\n",(int)rc_dig_in01);         will display 'Off 0'
*
*  3.  The %s directive only supports a string of 40 bytes and the format string
*      may not exceed 80 bytes.  The sprintf routine is not included.  Keep in 
*      mind that the stack size has a 256 byte limit.
*
* USAGE:
*  These library routines  may be modified to suit the needs of the user.
*******************************************************************************/

#include "ifi_default.h"
#include "printf_lib.h"
#include "ifi_utilities.h"
#include "string.h"

/* the following should be enough for a 32 bit word */
#define PRINT_BUF_LEN 20

extern char ifi_printfBufr[]; /* declared in ifi_utilities.c */
rom char *nullStr = "(null)";
static const rom char * bufrTooLongMsg = "**** printf format string too long, max is 80 ****\n";
static const rom char * s_formatTooLongMsg = "**** %s arg too long, limit 40 ****"; // make sure this error msg len < 40!

static char print_buf[PRINT_BUF_LEN];
static char scr[2];
static int  k;

unsigned short strlenrom(const rom char * string) 
{
	unsigned short len;
	for(len=0; string[len]!=0; len++) ;
	return len;
}

/*******************************************************************************
* SUBROUTINE NAME: Write_Byte_To_Uart
* PURPOSE:       Writes a byte to the UART.
*     Argument       Type           IO   Description
*     --------       -----------    --   -----------
*         data       int            I    data to transmit to the UART
* RETURNS:       void
*******************************************************************************/

static void Write_Byte_To_Uart(int data)
{
  TXREG = data;  /* a carriage return */
  Wait4TXEmpty();
}

#define PAD_RIGHT 1
#define PAD_ZERO  2

/*******************************************************************************
* FUNCTION NAME: prints
* PURPOSE:       Pads the output stream.
* RETURNS:       void
*******************************************************************************/

static int prints(char *string, int width, int pad)
{
	register int pc = 0, padchar = ' ';

	if (width > 0) {
		register int len = 0;
		register char *ptr;
		for (ptr = string; *ptr; ++ptr) ++len;
		if (len >= width) width = 0;
		else width -= len;
		if (pad & PAD_ZERO) padchar = '0';
	}
	if (!(pad & PAD_RIGHT)) {
		for ( ; width > 0; --width) {
			Write_Byte_To_Uart (padchar);
			++pc;
		}
	}
	for ( ; *string ; ++string) {
		Write_Byte_To_Uart (*string);
		++pc;
	}
	for ( ; width > 0; --width) {
		Write_Byte_To_Uart (padchar);
		++pc;
	}

	return pc;
}

/*******************************************************************************
* FUNCTION NAME: printi
* PURPOSE:       Converts the output stream to the proper width and base.
* RETURNS:       void
*******************************************************************************/

static int printi(int i, int b, int sg, int width, int pad, int letbase)
{
	register char *s;
	register int t, neg = 0, pc = 0;
	register unsigned int u = i;

   print_buf[0] = 0xBE;
   print_buf[PRINT_BUF_LEN] = 0xEF;
	if (i == 0) {
		print_buf[0] = '0';
		print_buf[1] = '\0';
		return prints (print_buf, width, pad);
	}

	if (sg && b == 10 && i < 0) {
		neg = 1;
		u = -i;
	}

	s = print_buf + PRINT_BUF_LEN-1;
	*s = '\0';

	while (u) {
		t = u % b;
		if( t >= 10 )
			t += letbase - '0' - 10;
		*--s = t + '0';
		u /= b;
	}

	if (neg) {
		if( width && (pad & PAD_ZERO) ) {
			Write_Byte_To_Uart ('-');
			++pc;
			--width;
		}
		else {
			*--s = '-';
		}
	}

	return pc + prints (s, width, pad);
}

/*******************************************************************************
* FUNCTION NAME: print
* PURPOSE:       Parses the output stream.
* RETURNS:       void
*******************************************************************************/

static int print(char *format, int *varg)
{
  char tmpBufr[40];
	register int width, pad;
	register int pc = 0;

	for (; *format != 0; ++format) {
		if (*format == '%') {
			++format;
			width = pad = 0;
			if (*format == '\0') break;
			if (*format == '%') goto out;
			if (*format == '-') {
				++format;
				pad = PAD_RIGHT;
			}
			while (*format == '0') {
				++format;
				pad |= PAD_ZERO;
			}
			for ( ; *format >= '0' && *format <= '9'; ++format) {
				width *= 10;
				width += *format - '0';
			}
			if( *format == 's' ) 
      {
		if(strlenrom((rom char *) *varg)<40)
	        strcpypgm2ram(tmpBufr,(rom char *) *varg);
		else
			strcpypgm2ram(tmpBufr, (rom char *) s_formatTooLongMsg);
		varg--;
        if (tmpBufr[0] == 0)
          strcpypgm2ram(tmpBufr,nullStr);
				pc += prints (tmpBufr, width, pad);
				continue;
			}
			if( *format == 'd' ) {
				pc += printi (*varg--, 10, 1, width, pad, 'a');
				continue;
			}
			if( *format == 'x' ) {
				pc += printi (*varg--, 16, 0, width, pad, 'a');
				continue;
			}
			if( *format == 'X' ) {
				pc += printi (*varg--, 16, 0, width, pad, 'A');
				continue;
			}
			if( *format == 'u' ) {
				pc += printi (*varg--, 10, 0, width, pad, 'a');
				continue;
			}
			if( *format == 'l' ) {    /* assumes lx */
				pc += printi (*varg--, 16, 0, width, pad, 'a');
				pc += printi (*varg--, 16, 0, width, pad, 'a');
				format++;               /* skip over x*/
				continue;
			}
			if( *format == 'b' ) {
				pc += printi (*varg--, 2, 0, width, 2, 'a');
				continue;
			}
		}
		else {
		out:
      if (*format == '\n') *format = '\r';  /* replace line feed with cr */
			Write_Byte_To_Uart (*format);
			++pc;
		}
	}
	return pc;
}

/*******************************************************************************
* FUNCTION NAME: printf
* PURPOSE:       Formats an output stream.
* RETURNS:       void
*******************************************************************************/
int printf(rom const char *format, ...)
{
    register int *varg = (int *)(&format);
    /* 
       Since constant strings are kept in program (flash) memory, the strcpypgm2ram
       routine copies the string from flash to ram. 
    */
	if(strlenrom(format)<printfBufrLength) 
	    strcpypgm2ram(ifi_printfBufr,(rom char *) format);
	else 	
		// format string too long, print error message instead
		strcpypgm2ram(ifi_printfBufr, (rom char *)bufrTooLongMsg);

    varg--; /* adjust stack for Microchip C Compiler */
    return print(ifi_printfBufr, varg);
}

/******************************************************************************
* 
* The following routines are examples how to use the printf library routines
* to create your own output modules:
*  
*******************************************************************************/

/*******************************************************************************
* FUNCTION NAME: printid
* PURPOSE:       Prints a 16bit word (base 10) w/o a carriage return.
* RETURNS:       void
*******************************************************************************/

void printid(int data,int crtOn)
{
  printi (data, 10, 1, 4, 2, 'a');
  if (crtOn)
    Write_Byte_To_Uart('\r');
}

/*******************************************************************************
* FUNCTION NAME: printd
* PURPOSE:       Prints an 8bit word (base 10) w/o a carriage return.
* RETURNS:       void
*******************************************************************************/

void printd(unsigned char data,int crtOn)
{
  printi ((int) data, 10, 1, 3, 2, 'a');
  if (crtOn)
    Write_Byte_To_Uart('\r');
}

/*******************************************************************************
* FUNCTION NAME: printib
* PURPOSE:       Prints a 16bit binary word (base 2) w/o a carriage return.
* RETURNS:       void
*******************************************************************************/

void printib(unsigned int data,int crtOn)
{
  printi (data, 2, 0, 16, 2, 'a');
  if (crtOn)
    Write_Byte_To_Uart('\r');
}

/*******************************************************************************
* FUNCTION NAME: printb
* PURPOSE:       Prints an 8bit binary word (base 2) w/o a carriage return.
* RETURNS:       void
*******************************************************************************/

void printb(unsigned char data,int crtOn)
{
  printi ((int) data, 2, 0, 8, 2, 'a');
  if (crtOn)
    Write_Byte_To_Uart('\r');
}

/*******************************************************************************
* FUNCTION NAME: printix
* PURPOSE:       Prints a 16bit hex word (base 16) w/o a carriage return.
* RETURNS:       void
*******************************************************************************/

void printix(int data,int crtOn)
{
  printi (data, 16, 0, 2, 0, 'a');
  if (crtOn)
    Write_Byte_To_Uart('\r');
}

/*******************************************************************************
* FUNCTION NAME: printx
* PURPOSE:       Prints an 8bit hex word (base 16) w/o a carriage return.
* RETURNS:       void
*******************************************************************************/

void printx(unsigned char data,int crtOn)
{
  printi ((int) data, 16, 0, 2, 2, 'a');
  if (crtOn)
    Write_Byte_To_Uart('\r');
}

/*******************************************************************************
* FUNCTION NAME: debug_print
* PURPOSE:       Prints a header and a 16bit hex word (base 16) with a carriage 
*                return.
* RETURNS:       void
*******************************************************************************/

void debug_print(char *bufr,int data)
{
  if(strlenrom(bufr)<printfBufrLength) 
     strcpypgm2ram(ifi_printfBufr,(rom char *) bufr);
  else 	
    // format string too long, print error message instead
	strcpypgm2ram(ifi_printfBufr, (rom char *)bufrTooLongMsg);
  for (k=0;k<strlen(ifi_printfBufr);k++)
    Write_Byte_To_Uart(ifi_printfBufr[k]);
  printix(data,1);
}

/*******************************************************************************
* FUNCTION NAME: debug_printb
* PURPOSE:       Prints a header and an 8bit binary word (base 2) with a carriage 
*                return.
* RETURNS:       void
*******************************************************************************/

void debug_printb(char *bufr,unsigned int data)
{
  if(strlenrom(bufr)<printfBufrLength) 
     strcpypgm2ram(ifi_printfBufr,(rom char *) bufr);
  else 	
    // format string too long, print error message instead
	strcpypgm2ram(ifi_printfBufr, (rom char *)bufrTooLongMsg);
  for (k=0;k<strlen(ifi_printfBufr);k++)
    Write_Byte_To_Uart(ifi_printfBufr[k]);
  printib(data,1);
}

/*******************************************************************************
* FUNCTION NAME: debug_println
* PURPOSE:       Prints a header (assumming a carriage return is supplied).
* RETURNS:       void
*******************************************************************************/

void debug_println(char *bufr)
{
  if(strlenrom(bufr)<printfBufrLength) 
     strcpypgm2ram(ifi_printfBufr,(rom char *) bufr);
  else 	
    // format string too long, print error message instead
	strcpypgm2ram(ifi_printfBufr, (rom char *)bufrTooLongMsg);
  for (k=0;k<strlen(ifi_printfBufr);k++)
    Write_Byte_To_Uart(ifi_printfBufr[k]);
}


/******************************************************************************/
/******************************************************************************/
/******************************************************************************/

[Mark McLeod]

Originally Posted by gwross

Bill and Mark, if you could share your code, it would be appreciated.

Here's mine.

I didn't include some related minor changes made to the ifi_utilities functions.
We added "0x" in front of the hex prints and added a compile flag "_NO_PRINTS", so we could easily turn on/off all prints.

We kept the 80 byte limitation, but truncate strings at 80 bytes. I suppose I could have added "..." or something at the end of a truncated string to indicate when a text string is too long, but this code doesn't create one huge string.
It also doesn't care about the size of the variable being printed. There are separate calls for printing signed vs unsigned integers, and I never paid any attention to floats.
It's inefficient in that it outputs in discrete chunks.

I made it as simple as I could, so the newbie programmers could more easily follow the logic.
Send me any improvements you think of.