Concatenate with memcpy

Question

I'm trying to add two strings together using memcpy. The first memcpy does contain the data, I require. The second one does not however add on. 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++;
}

FYI: it is permitted in C to break long statements onto multiple lines, or even into multiple statements with extra variables to hold intermediate results. — James McNellis, Aug 03 '11 at 18:17
I'd note that some embedded databases (berkeley DB is one I think) use data and length explicitly. Data isn't required to be null terminated and may contain nulls. In which case, if your program already got the length from the DB API, keep it around and avoid using strlen. — Zan Lynx, Feb 21 '13 at 20:17

asveikau · Answer 1 · 2011-08-03T18:51:24.530

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 gains you the following:

Less redundant calls to strlen. Each of those must traverse the string, so it's a good idea to minimize these calls.
The 2nd memcpy needs to actually append, not replace. So the first argument has to differ from the previous call.
Note the +1 in the 3rd arg of the 2nd memcpy. That is for the NUL terminator.

I'm not sure your if statement makes sense either. Perhaps a more sane thing to do would be to make sure that g->db_cmd has enough space for what you are about to copy. You would do that via either sizeof (if db_cmd is an array of characters) or by tracking how big your heap allocations are (if db_cmd was acquired via malloc). So perhaps it would make most 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.
}

score 1 · Answer 2 · answered Aug 03 '11 at 18:24

You're not copying the null terminator, you're only coping the raw string data. That leaves your string non-null-terminated, which can cause all sorts of problems. You're also not checking to make sure you have enough space in your buffer, which can result in buffer overflow vulnerabilities.

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

Michael Burr · Answer 3 · 2011-08-03T18:58:39.677

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

So when strlen(g->db_cmd) is called in the second call to memcpy() it might be returning something completely bogus. Whether this is a problem depends on whether the g->db_cmd buffer is initialized to zeros beforehand or not.

Why not use the strcat(), which was made to do exactly what you're 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++;
     }

That'll have the advantage of being easier for someone to read. You might think it would be less performant - but I don't think so since you're making a bunch of strlen() calls explicitly. In any case, I'd concentrate on getting it right first, then worry about performance. Incorrect code is as unoptimized as you can get - get it right before getting it fast. In fact, my next step wouldn't be to improve the code performance-wise, it would be to improve the code to be less likely to have a buffer overrun (I'd 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:

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() isn't part of your platform it can be found on the net pretty easily. If you're using MSVC there's a strcat_s() function which you could use instead (but note that it's not equivalent to strlcat() - you'd have to change how the results from calling strcat_s() are checked and handled).

Concatenate with memcpy

3 Answers3

Linked