Solved

cheking allocated size for a buffer passed in parameter to a function

Posted on 2010-09-03
23
387 Views
Last Modified: 2012-05-10
Hi,

I have a function but I don't know if the way I check the passed parameter to it is secure ?

IS the data_size variable used efficiently?
Must I use  a temp_buf or it is not matter to use directly the calib_data  ?

See code below.

Thank you

#include <stdio.h>

#include <string.h>

#include <malloc.h>



#define ERROR_CALIB_DATA_SIZE   -5



#define MAX_DATA_SIZE			450

#define MAX_MATRIX_BUFFER		450







//################################################################################################

//				

//  IN      : int data_size: size of the calibration data buffer 

//	IN/OUT	: char * calib_data: the formatted calibration data buffer (max size = 450 bytes)

//################################################################################################

int compose_data(int p_A_0_xi, int p_A_0_yi, int p_A_Z_xi, int p_A_Z_yi, int p_B_0_xi,

  int p_B_0_yi, int p_B_Z_xi, int p_B_Z_yi, int height_cm, double xy,int res_x, int res_y, int data_size, char * calib_data)

{

   int ret = 0;



   int nchars = 0;

   int real_size   = 0;

   char temp_buf[MAX_DATA_SIZE]={0};



   sprintf( temp_buf,"PA_0_X:%d;PA_0_Y:%d;PA_Z_X:%d;PA_Z_Y:%d;PB_0_X:%d;PB_0_Y:%d;PB_Z_X:%d;PB_Z_Y:%d\nHEIGHT_CM:%d;PIXEL_XY:%f;RES_X:%d;RES_Y:%d\n", 

      p_A_0_xi,  p_A_0_yi,  p_A_Z_xi,  p_A_Z_yi, 

      p_B_0_xi,  p_B_0_yi,  p_B_Z_xi,  p_B_Z_yi, 

      height_cm,  xy, res_x, res_y);



   real_size =strlen(temp_buf);



   if (real_size>data_size)

	   return ERROR_CALIB_DATA_SIZE;



   strcpy (calib_data,temp_buf);



   return ret;

}





int main(int argc, char* argv[])

{



	int res_x = 5;

	int res_y = 15;

	char calib_data[MAX_DATA_SIZE]={0};

	char result_buffer[MAX_MATRIX_BUFFER]={0};



	int size_length = MAX_DATA_SIZE;

	printf("\nCalling vgt_compose_calib_data...\n");

	compose_data(70,170,110,210,50,90,150,120,170,1.0,res_x,res_y,size_length, calib_data);



	printf("STRING DATA BUFFER:\n%s\n",calib_data);

	int size_in = strlen(calib_data);//or sizeof(buffer)

	printf("size of calib_data: %d\n",size_in);



	getchar();



return 0;

}

Open in new window

0
Comment
Question by:Develprog
  • 9
  • 7
  • 5
  • +1
23 Comments
 
LVL 33

Expert Comment

by:pgnatyuk
ID: 33596815
Too many parameters. You can wrap your data in few structures. So the function will have 2-3 parameters. If it's possible.

 Instead of sprintf, you can use snprintf (_snprintf) - to control that the string is less than MAX_DATA_SIZE. Or data_size. It will allow you to avoid this check:
 if (real_size>data_size)
           return ERROR_CALIB_DATA_SIZE;

and the temporary buffer can be removed.

0
 
LVL 40

Expert Comment

by:evilrix
ID: 33597000
>> iS the data_size variable used efficiently?
What do you mean by efficiently?


>> Must I use  a temp_buf or it is not matter to use directly the calib_data  ?
You could probably lose temp_buf by using snprintf if your compiler supports it.
http://www.cppreference.com/wiki/c/io/snprintf
0
 
LVL 53

Accepted Solution

by:
Infinity08 earned 200 total points
ID: 33597319
Since you're asking about performance, that's what I'll comment on. I will ignore maintainability and other such concerns - under the assumption that you will properly keep them in mind.

>>    real_size =strlen(temp_buf);

sprintf already returns the number of characters in the string, so there's no need to call strlen.


>>    if (real_size>data_size)

You're not taking the terminating '\0' character into account here ! You need to make sure that there's enough room for that too in the calib_data buffer.


>>       char calib_data[MAX_DATA_SIZE]={0};

>>    char temp_buf[MAX_DATA_SIZE]={0};

If your temporary buffer inside the compose_data function has the same size as the buffer you're passing in as a parameter - what's the point ? You're performing unnecessary memory copies.



>> #define MAX_DATA_SIZE                  450

That's more than enough for the data you're placing in the string (assuming 32bit integers).

A 32-bit int printed using %d has a maximum width of 11 characters.
A double printed using %f can theoretically be quite wide, but usually has a reasonable upper limit. Count 20 characters just to be safe.

So, the format "PA_0_X:%d;PA_0_Y:%d;PA_Z_X:%d;PA_Z_Y:%d;PB_0_X:%d;PB_0_Y:%d;PB_Z_X:%d;PB_Z_Y:%d\nHEIGHT_CM:%d;PIXEL_XY:%f;RES_X:%d;RES_Y:%d\n" results in max. (19 * 8) + 22 + 30 + 18 + 18 + 1 = 241 characters.
Given that (and assuming the upper bound is adapted whenever the format changes), you don't need to worry about whether there's enough room in the buffer (as long as the buffer is at least as big as the upper bound).
0
 

Author Comment

by:Develprog
ID: 33597320

In my case I must not use snprintf()

>>What do you mean by efficiently?
in fact I set data_size variable by this way : int size_length = MAX_DATA_SIZE;
because  the calib_data contain is 0 otherwhise I 'll use strlen() for example.


And the final goal is to avoid overflow so why  I used a temporary buffer, is it correct ?

So if I use like in this code below is it ok to avoid any overflow problem?

Thank you

 
int compose_data(int p_A_0_xi, int p_A_0_yi, int p_A_Z_xi, int p_A_Z_yi, int p_B_0_xi,

  int p_B_0_yi, int p_B_Z_xi, int p_B_Z_yi, int height_cm, double xy,int res_x, int res_y, int data_size, char * calib_data)

{

   int ret = 0;



   int real_size   = 0;

   

   sprintf( calib_data,"PA_0_X:%d;PA_0_Y:%d;PA_Z_X:%d;PA_Z_Y:%d;PB_0_X:%d;PB_0_Y:%d;PB_Z_X:%d;PB_Z_Y:%d\nHEIGHT_CM:%d;PIXEL_XY:%f;RES_X:%d;RES_Y:%d\n", 

      p_A_0_xi,  p_A_0_yi,  p_A_Z_xi,  p_A_Z_yi, 

      p_B_0_xi,  p_B_0_yi,  p_B_Z_xi,  p_B_Z_yi, 

      height_cm,  xy, res_x, res_y);

	  

   real_size =strlen(calib_data);



   if (real_size>data_size)

	   return ERROR_CALIB_DATA_SIZE;



   return ret;

}

Open in new window

0
 
LVL 33

Assisted Solution

by:pgnatyuk
pgnatyuk earned 50 total points
ID: 33597416
calib_data now is an input parameter. So it can be 0.

This function look very bad. The overflow is the smallest problem here - I think so.

0
 

Author Comment

by:Develprog
ID: 33597635

Thank you Infinity08 for detailed informations.

Yes pgnatyuk, it will be better I presume to initialize calib_data like this : memset(calib_data,0,data_size);

In fact the function compose_data() is a dll function

And the call (client) must know the size to allocate for calib_data which is an input and output parameter for the compose_data() function.
And the fucntion must know too the size of calib_data to check if result of sprintf can be putted in.
 
But the problem is how chek it correctly to avoid any overflow ?

Thank you



0
 
LVL 33

Expert Comment

by:pgnatyuk
ID: 33597670
This function has too many parameters, so it looks like it looks.
It's supposed to convert a set of parameters into a string. It uses a temporary memory buffer. Probably, in order to avoid a memory overrun. Ok.
int compose_data(int p_A_0_xi, int p_A_0_yi, int p_A_Z_xi, int p_A_Z_yi, int p_B_0_xi,
				 int p_B_0_yi, int p_B_Z_xi, int p_B_Z_yi, int height_cm, 
				 double xy,int res_x, int res_y, int data_size, char * calib_data)
{
	int ret = ERROR_CALIB_DATA_SIZE;
	int real_size   = 0;
	char temp_buf[MAX_DATA_SIZE] = { 0 };
	
	if (calib_data == 0 || data_size == 0)
		return ret;

	real_size = sprintf( temp_buf,"PA_0_X:%d;PA_0_Y:%d;PA_Z_X:%d;PA_Z_Y:%d;PB_0_X:%d;PB_0_Y:%d;PB_Z_X:%d;PB_Z_Y:%d\nHEIGHT_CM:%d;PIXEL_XY:%f;RES_X:%d;RES_Y:%d\n", 
			p_A_0_xi,  p_A_0_yi,  p_A_Z_xi,  p_A_Z_yi, 
			p_B_0_xi,  p_B_0_yi,  p_B_Z_xi,  p_B_Z_yi, 
			height_cm,  xy, res_x, res_y);
	
	if (real_size < data_size)
	{
		strcpy (calib_data, temp_buf);
		ret = 0;
	}
	
	return ret;
}

Open in new window

0
 
LVL 53

Expert Comment

by:Infinity08
ID: 33597694
>> But the problem is how chek it correctly to avoid any overflow ?

Compare the size to the upper bound and/or use snprintf (as has been suggested earlier).
0
 
LVL 33

Expert Comment

by:pgnatyuk
ID: 33597719
You can use a small Windows trick - there are many function that returns the size. For example, when I need to convert a multibyte character text to ascii, I call the MultiByteToWideChar function with NULL as the output parameter and in this case the function will return the required size. Take a look.

int compose_data(int p_A_0_xi, int p_A_0_yi, int p_A_Z_xi, int p_A_Z_yi, int p_B_0_xi,
				 int p_B_0_yi, int p_B_Z_xi, int p_B_Z_yi, int height_cm, 
				 double xy,int res_x, int res_y, int data_size, char * calib_data)
{
	int real_size   = 0;
	char temp_buf[MAX_DATA_SIZE] = { 0 };
	
	real_size = sprintf( temp_buf,"PA_0_X:%d;PA_0_Y:%d;PA_Z_X:%d;PA_Z_Y:%d;PB_0_X:%d;PB_0_Y:%d;PB_Z_X:%d;PB_Z_Y:%d\nHEIGHT_CM:%d;PIXEL_XY:%f;RES_X:%d;RES_Y:%d\n", 
			p_A_0_xi,  p_A_0_yi,  p_A_Z_xi,  p_A_Z_yi, 
			p_B_0_xi,  p_B_0_yi,  p_B_Z_xi,  p_B_Z_yi, 
			height_cm,  xy, res_x, res_y);
	
	if (calib_data != 0 && real_size < data_size)
		strcpy (calib_data, temp_buf);
	
	return real_size;
}

Open in new window

0
 
LVL 40

Expert Comment

by:evilrix
ID: 33597754
>> In my case I must not use snprintf()

Any specific reason why?
0
 
LVL 33

Expert Comment

by:pgnatyuk
ID: 33597787
In Windows it's _snprintf, on Mac it's snprintf - that's one example. Use #ifdef for such trivial case.... it will not make the code look&feel better.
0
Threat Intelligence Starter Resources

Integrating threat intelligence can be challenging, and not all companies are ready. These resources can help you build awareness and prepare for defense.

 

Author Comment

by:Develprog
ID: 33598056

I'm not using snprintf() only to use more standard functions an
more for using other tips.

Ok the code here below work well.

Thank you


int compose_data(int p_A_0_xi, int p_A_0_yi, int p_A_Z_xi, int p_A_Z_yi, int p_B_0_xi,

  int p_B_0_yi, int p_B_Z_xi, int p_B_Z_yi, int height_cm, double pixel_xy,int res_x, int res_y, int data_size, char * calib_data)

{

   int ret = 0;



   int nchars = 0;

   int real_size   = 0;

   char temp_buf[MAX_CALIBRATION_DATA]={0};

   memset(calib_data,0,data_size);



   real_size=sprintf( temp_buf,"PA_0_X:%d;PA_0_Y:%d;PA_Z_X:%d;PA_Z_Y:%d;PB_0_X:%d;PB_0_Y:%d;PB_Z_X:%d;PB_Z_Y:%d\nHEIGHT_CM:%d;PIXEL_XY:%f;RES_X:%d;RES_Y:%d\n", 

      p_A_0_xi,  p_A_0_yi,  p_A_Z_xi,  p_A_Z_yi, 

      p_B_0_xi,  p_B_0_yi,  p_B_Z_xi,  p_B_Z_yi, 

      height_cm,  pixel_xy, res_x, res_y);



   if (calib_data != 0 && real_size < data_size)

		strcpy (calib_data,temp_buf);

   else

	   return ERROR_CALIB_DATA_SIZE;

   

   return ret;

}

Open in new window

0
 
LVL 53

Expert Comment

by:Infinity08
ID: 33598206
You're still doing a lot of unnecessary stuff.
You're still not accounting for the terminating '\0' character pcorrectly.
You're still in danger of a segmentation fault if calib_data is NULL.
0
 

Author Comment

by:Develprog
ID: 33609660


>>You're still not accounting for the terminating '\0' character pcorrectly.


How must I checked it after sprintf function and how ?

Thank you
0
 
LVL 53

Expert Comment

by:Infinity08
ID: 33609829
>>    if (calib_data != 0 && real_size < data_size)
>>             strcpy (calib_data,temp_buf);

real_size contains the number of characters in the string, WITHOUT the terminating '\0' character.
data_size is the size of the buffer.
strcpy copies (real_size + 1) characters to that buffer (ie. INCLUDING the terminating '\0' character).

So, the terminating '\0' character has to be taken into account when checking whether the buffer is big enough to hold the entire string.


That said, I didn't notice that you changed the '>' to a '<', so the check you currently have should be ok.

The other points I brought up, still apply though ...
0
 

Author Comment

by:Develprog
ID: 33610292

>> You're still in danger of a segmentation fault if calib_data is NULL

yes but how verifying that in the function? Normally the calib_data variable is initialize before passed to the function


Thank you

0
 
LVL 53

Expert Comment

by:Infinity08
ID: 33610722
You can check whether it's NULL before using it.
0
 

Author Comment

by:Develprog
ID: 33610803

so it will be like code below ?


but this cheking was too did after the sprintf function like here:

...
   if (calib_data != 0 && real_size < data_size)
            strcpy (calib_data,temp_buf);
...


Is it good to check it too after the sprintf function like already did?

Thank you

{

   int ret = 0;



   int nchars = 0;

   int real_size   = 0;

   char temp_buf[MAX_CALIBRATION_DATA]={0};

   if(calib_data!=NULL)  //NEW CHECK

	   return ERROR_BAD_INITIALIZED_BUFFER;

   memset(calib_data,0,data_size);



   real_size=sprintf( temp_buf,"PA_0_X:%d;PA_0_Y:%d;PA_Z_X:%d;PA_Z_Y:%d;PB_0_X:%d;PB_0_Y:%d;PB_Z_X:%d;PB_Z_Y:%d\nHEIGHT_CM:%d;PIXEL_XY:%f;RES_X:%d;RES_Y:%d\n", 

      p_A_0_xi,  p_A_0_yi,  p_A_Z_xi,  p_A_Z_yi, 

      p_B_0_xi,  p_B_0_yi,  p_B_Z_xi,  p_B_Z_yi, 

      height_cm,  pixel_xy, res_x, res_y);



   if (calib_data != 0 && real_size < data_size)

		strcpy (calib_data,temp_buf);

   else

	   return ERROR_CALIB_DATA_SIZE;

   

   return ret;

}

Open in new window

0
 

Author Comment

by:Develprog
ID: 33610826

Oh Sorry,


 this line :

 if(calib_data!=NULL)  //NEW CHECK

will be changed to:

 if(calib_data==NULL)  //NEW CHECK


0
 
LVL 53

Expert Comment

by:Infinity08
ID: 33610848
>> but this cheking was too did after the sprintf function like here:

Yes, but that check was too late, because you were already de-referencing the pointer before it.


>> Is it good to check it too after the sprintf function like already did?

There's no reason to do that - it'll just consume some unnecessary CPU cycles.


>>    if(calib_data!=NULL)  //NEW CHECK

Here you're checking if it's NOT NULL, and you return an error if that's the case. You want to return an error if it IS NULL.



Furthermore, as I said earlier - there's still a lot of work for no reason. The memset doesn't serve a purpose eg. There are a few local variables that aren't used or shouldn't be used. And I would get rid of the temporary buffer like has been suggested before.
0
 

Author Comment

by:Develprog
ID: 33611028

actually the calib_data is initialized before calling the function like this:


...

char calib_data[MAX_CALIBRATION_DATA]={0};

....

compose_data(70,170,110,210,50,90,150,120,170,1.0,...., calib_data);

....
0
 
LVL 53

Assisted Solution

by:Infinity08
Infinity08 earned 200 total points
ID: 33611066
>> actually the calib_data is initialized before calling the function like this:

Yes, but the function doesn't know that. What if the function is called from elsewhere in the code, where that initialization is not done ?

Either you have to document the function that NULL cannot be passed, or it will crash. Or you have to explicitly check for NULL. The latter is preferred, because it's safer.
0
 

Author Comment

by:Develprog
ID: 33625321

Ok it is better now, thank you all
0

Featured Post

Highfive + Dolby Voice = No More Audio Complaints!

Poor audio quality is one of the top reasons people don’t use video conferencing. Get the crispest, clearest audio powered by Dolby Voice in every meeting. Highfive and Dolby Voice deliver the best video conferencing and audio experience for every meeting and every room.

Join & Write a Comment

A short article about problems I had with the new location API and permissions in Marshmallow
Although it can be difficult to imagine, someday your child will have a career of his or her own. He or she will likely start a family, buy a home and start having their own children. So, while being a kid is still extremely important, it’s also …
The goal of this video is to provide viewers with basic examples to understand and use conditional statements in the C programming language.
In this fourth video of the Xpdf series, we discuss and demonstrate the PDFinfo utility, which retrieves the contents of a PDF's Info Dictionary, as well as some other information, including the page count. We show how to isolate the page count in a…

706 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

18 Experts available now in Live!

Get 1:1 Help Now