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;

>> 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

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

There are many ways to learn to code these days. From coding bootcamps like Flatiron School to online courses to totally free beginner resources. The best way to learn to code depends on many factors, but the most important one is you. See what course is best for you.

>>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;}

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 ?

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;}

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;}

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;}

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.

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 ...

>> 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.

>> 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.

What do responsible coders do? They don't take detrimental shortcuts. They do take reasonable security precautions, create important automation, implement sufficient logging, fix things they break, and care about users.