Solved

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

Posted on 2010-09-03
23
395 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
Gigs: Get Your Project Delivered by an Expert

Select from freelancers specializing in everything from database administration to programming, who have proven themselves as experts in their field. Hire the best, collaborate easily, pay securely and get projects done right.

 

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
 

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

Gigs: Get Your Project Delivered by an Expert

Select from freelancers specializing in everything from database administration to programming, who have proven themselves as experts in their field. Hire the best, collaborate easily, pay securely and get projects done right.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

Title # Comments Views Activity
splitOdd10 challenge 5 104
MacOS and programming in React 7 34
nested if statement in excel help 4 26
If a cell in a range equals "YES" return specific Test. 4 27
Displaying an arrayList in a listView using the default adapter is rarely the best solution. To get full control of your display data, and to be able to refresh it after editing, requires the use of a custom adapter.
This article will inform Clients about common and important expectations from the freelancers (Experts) who are looking at your Gig.
The goal of this video is to provide viewers with basic examples to understand and use structures in the C programming language.
In this fifth video of the Xpdf series, we discuss and demonstrate the PDFdetach utility, which is able to list and, more importantly, extract attachments that are embedded in PDF files. It does this via a command line interface, making it suitable …

785 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