Combine with memcpy

Question

I am trying to add two lines together with memcpy. The first memcpy contains the data I need. The second, however, does not add. Any idea why?

if (strlen(g->db_cmd) < MAX_DB_CMDS ) { memcpy(&g->db_cmd[strlen(g->db_cmd)],l->db.param_value.val,strlen(l->db.param_value.val)); memcpy(&g->db_cmd[strlen(g->db_cmd)],l->del_const,strlen(l->del_const)); g->cmd_ctr++; } 

Answer 1

 size_t len = strlen(l->db.param_value.val); memcpy(g->db_cmd, l->db.param_value.val, len); memcpy(g->db_cmd + len, l->del_const, strlen(l->del_cost)+1); 

This will result in the following:

• Less redundant strlen calls. Each of them must cross the line, so it is recommended to minimize these calls.
• The second memcpy should actually be added, not replaced. Therefore, the first argument must be different from the previous call.
• Note the +1 in the third arg of the second memcpy . That is, for the NUL terminator.

I'm not sure if your if makes sense. Perhaps a more reasonable task would be to make sure that g->db_cmd has enough space for what you are going to copy. You can do this via sizeof (if db_cmd is an array of characters) or by tracking how large your heap allocations are (if db_cmd was obtained through malloc ). Therefore, perhaps this would make sense as:

 size_t param_value_len = strlen(l->db.param_value.val), del_const_len = strlen(l->del_const); // Assumption is that db_cmd is a char array and hence sizeof(db_cmd) makes sense. // If db_cmd is a heap allocation, replace the sizeof() with how many bytes you // asked malloc for. // if (param_value_len + del_const_len < sizeof(g->db_cmd)) { memcpy(g->db_cmd, l->db.param_value.val, param_value_len); memcpy(g->db_cmd + param_value_len, l->del_const, del_const_len + 1); } else { // TODO: your buffer is not big enough. handle that. } 

Answer 2

You are not copying the null terminator; you are only processing raw string data. This leaves a line with zero completion, which can cause all kinds of problems. You also do not verify that you have enough space in your buffer, which can lead to buffer overflow vulnerabilities .

To make sure you copy the null terminator, just add 1 to the number of copies you are copying - copy strlen(l->db.param_value.val) + 1 bytes.

Answer 3

One possible problem is that your first memcpy() call will not necessarily result in a null terminated string, since you are not copying the terminator '\ 0' from l->db.param_value.val :

Therefore, when strlen(g->db_cmd) is called in the second memcpy() call, it can return something completely fake. Whether this is a problem depends on whether the g->db_cmd initialized with zeros in advance or not.

Why not use strcat() , which was done in order to do exactly what you are trying to do with memcpy() ?

 if (strlen(g->db_cmd) < MAX_DB_CMDS ) { strcat( g->db_cmd, l->db.param_value.val); strcat( g->db_cmd, l->del_const); g->cmd_ctr++; } 

This will have the advantage of making it easier for someone to read. You might think that it will be less efficient, but I don’t think so, since you are doing a bunch of strlen() calls explicitly. In any case, I would focus on getting everything right, and then worry about performance. An incorrect code is just as unoptimized as you can get - get it right before you get it quickly. In fact, my next step would be not to improve the code, but to improve the code so that it is less likely to have a buffer overflow (I would probably switch to using something like strlcat() instead of strcat() ).

For example, if g->db_cmd is a char array (and not a pointer), the result might look like this:

 size_t orig_len = strlen(g->db_cmd); size_t result = strlcat( g->db_cmd, l->db.param_value.val, sizeof(g->db_cmd)); result = strlcat( g->db_cmd, l->del_const, sizeof(g->db_cmd)); g->cmd_ctr++; if (result >= sizeof(g->db_cmd)) { // the new stuff didn't fit, 'roll back' to what we started with g->db_cmd[orig_len] = '\0'; g->cmd_ctr--; } 

If strlcat() not part of your platform, it can be easily found on the net. If you use MSVC, there is a strcat_s() function there that you could use instead (but note that it is not equivalent to strlcat() ), you will have to change the way you check and process the results when strcat_s() called).