CODEWITHSUNDEEP
c
user1621581
user1621581

Reputation: 113

strlcpy or memcpy?

I know strcpy is not secure when dealing with buffers, however in my code i have this

static char    buf[HUGE_BUFFER_SIZE + 1];
char servername[200];
int length = 0;
char *out = NULL;
        length = strlen(buf);
        out = alloca(strlen(servername) + length + 5);
        length = strlen(servername);
        strcpy(out, servername);
        out[length] = ' ';
        out[length + 1] = 0;
        strcat(out, buf);

If I change strcpy to strlcpy and use a sizeof for arg3, the output is garbled. Is there an easier way than using memcpy (which looks like I might have to do?) Im trying to avoid using insecure functions like strcpy and friends. Is there an easier way to accomplish the above with less copying of functions, and strnlens/alloca?

Upvotes: 0

Views: 2692

Answers (3)

vaxherd
vaxherd

Reputation: 116

It might be simpler to just use snprintf():

char out[1000];  // Or however large you need.
int len = snprintf(out, sizeof(out), "%s %s", servername, buf);
if (len >= sizeof(out)) {
    // Result would have overflowed the buffer and has been truncated.
}

Note >= instead of > in the conditional.

snprintf() returns the length of the text written, not including the terminating null byte. So, if the returned value is equal to the buffer size, that means the final character got truncated (because snprintf() always null-terminates the resulted string).

Upvotes: 2

Savanni D'Gerinel
Savanni D'Gerinel

Reputation: 2489

I'd do it like this:

#include <string.h>
#include <stdio.h>
#include <stdlib.h>

#define HUGE_BUFFER_SIZE 4096

int main (int argc, char **argv) {
  static char buf[HUGE_BUFFER_SIZE + 1];
  char servername[200];
  int target_len, servername_len, buf_len = 0;
  char *out = NULL;

  strcpy(buf, "this is my test buffer!");
  strcpy(servername, "127.0.0.1");

  /* Notice the uses of strnlen.  Each of these fixes the maximum number of characters that will be scanned. */
  servername_len = strnlen(servername, HUGE_BUFFER_SIZE+1) + 1;
  buf_len = strnlen(buf, 200) + 4;
  target_len = servername_len, buf_len;

  if ((out = alloca(target_len)) == NULL)
    return EXIT_FAILURE; /* alloc failed, die quickly! */

  /* At this point, you've measured the length of servername and buf using strlen, so it should really be impossible to run beyond the length of your buffer, but we're going to be careful, anyway */
  out[0] = '\0';
  strncat(out, servername, servername_len);
  strncat(out, buf, buf_len);

  printf("%s\n", out);

  return EXIT_SUCCESS;
}

(barring a copy/paste error, or different libraries on your system and a cross-platform screwup on my part, you should be able to paste this into a file, compile it, and run it, and see "127.0.0.1this is my test buffer!" as the output)

Note that all of the str functions, whether strcpy, strcat, strncpy, or strlcpy, drop a null terminator onto the end of any string they create. The only time you need to manually put on a null terminator is when you are starting a brand new string, as I did with the statement "out[0] = '\0'".

Also, note that I used strnlen and strncat, not strlcat. I cannot tell from the documentation the difference between strlcat and strncat except in what the function returns.

Anyways, you should be able to avoid buffer-overrun security holes just be clearly bounding all of your function calls. strncpy and strncat bound how much data you will copy. strnlen bounds how long the system will search for a '\0'.

Edit: my original solution actually had an error that valgrind revealed. I was not initializing either buf or servername. While something else seemed to be magically initializing buf, valgrind reported that calling strcat with servername as the target was causing decisions to be made based on uninitialized data. doh.

Since I'm not sure why I was calling strcat to initialize my variables with string literals, I switched to strcpy. That, of course, is perfectly safe here because I have hard-coded into the application exactly the string I'm copying into the variable.

Upvotes: 1

Jay
Jay

Reputation: 24915

I think the reason why you must be getting garbled output is because you may be using sizeof for out. But, out is not a buffer and is a pointer and so you may be effetively passing only 4 (or whatever is the size of the pointer) as the size to the third argument, there by not copying more than 3 characters and rest is the unintialized memory which shows you garbage

You can try using strncpy and pass the length to be copied as the third argument.

Upvotes: 0

Related Questions