CODEWITHSUNDEEP
c
Tom Kadwill
Tom Kadwill

Reputation: 1478

Problems with strncpy and how to fix it

I am learning C and reading through Learn C The Hard Way (ISBN-10: 0-321-88492-2). I am stuck on the exercise 17 'How to break it'.

Here is the problem from the book:

There is a bug in this program because of strncpy being poorly designed. Go read about strncpy then try to find out what happens when the name or address you give is greater than 512 bytes. Fix this by simply forcing the last character to '\0' so that it's always set no matter what (which is what strncpy should do).

I have done some reading on strncpy and I understand that it is insecure because it doesn't add a null byte to the end of the string. However, I don't know how to pass in a large number of bytes to the function and I am not sure how to fix the null byte problem.

Below is the function which is using strncpy, MAX_DATA is set to 512.

void Database_set(struct Connection *conn, int id, const char *name, const char *email)
{
    struct Address *addr = &conn->db->rows[id];
    if(addr->set) die("Already set, delete it first");

    addr->set = 1;
    // WARNING: bug, read the "How To Break It" and fix this
    char *res = strncpy(addr->name, name, MAX_DATA);
    // demonstrate the strncpy bug
    if(!res) die("Name copy failed");

    res = strncpy(addr->email, email, MAX_DATA);
    if(!res) die("Email copy failed");
}

How to Break it - EDIT

Below is an example of how to break strncpy:

void Database_set(struct Connection *conn, int id, const char *name, const char *email)
{
    struct Address *addr = &conn->db->rows[id];
    if(addr->set) die("Already set, delete it first");

    addr->set = 1;
    // WARNING: bug, read the "How To Break It" and fix this

    char name2[] = {
      'a', 's', 't',
      'r', 'i', 'n', 'g'
    };
    char *res = strncpy(addr->name, name2, MAX_DATA);
    // demonstrate the strncpy bug
    if(!res) die("Name copy failed");

    res = strncpy(addr->email, email, MAX_DATA);
    if(!res) die("Email copy failed");
}

To fix, add a null byte to the end of the string. change name2 to be:

  char name2[] = {
    'a', 's', 't',
    'r', 'i', 'n', 'g', '\0'
  };

or, add the following line above the strncpy function call

names2[sizeof(names2)-1] = '\0';

Upvotes: 3

Views: 7704

Answers (5)

daniel
daniel

Reputation: 101

The main problem there is that addr->name has not been initialized, so it is an empty pointer, it points no where.

So before you can use strncpy first you have to allocate memory to addr->name, otherwise it won't work.

And since it is a NULL pointer, NULL gets returned if you didn't set it up, then the if statement after will be true and the die function will stop the program.

You can look at the source code that the Database_create function does not initialize the two string pointers from the struct Address.

Upvotes: 0

handles
handles

Reputation: 7853

The man page for strncpy actually gives example code on how to fix this bug:

strncpy(buf, str, n);
if (n > 0)
   buf[n - 1]= '\0';

Upvotes: 3

user3177100
user3177100

Reputation:

Why not just replace strncpy with strlcpy? According to the strlcpy man page :

 EXAMPLES
 The following sets chararray to ``abc\0\0\0'':

       (void)strncpy(chararray, "abc", 6);

 The following sets chararray to ``abcdef'' and does not NUL terminate
 chararray because the length of the source string is greater than or
 equal to the length parameter.  strncpy() only NUL terminates the
 destination string when the length of the source string is less than the
 length parameter.

       (void)strncpy(chararray, "abcdefgh", 6);

 Note that strlcpy(3) is a better choice for this kind of operation.  The
 equivalent using strlcpy(3) is simply:

       (void)strlcpy(buf, input, sizeof(buf));


 The following copies as many characters from input to buf as will fit and
 NUL terminates the result.  Because strncpy() does not guarantee to NUL
 terminate the string itself, it must be done by hand.

       char buf[BUFSIZ];

       (void)strncpy(buf, input, sizeof(buf) - 1);
       buf[sizeof(buf) - 1] = '\0';

Upvotes: 3

Olaf Dietsche
Olaf Dietsche

Reputation: 74028

Another way to fix the strncpy bug, would be to fix the printf call 

void Address_print(struct Address *addr)
{
    printf("%d %.*s %.*s\n",
            addr->id, sizeof(addr->name), addr->name, sizeof(addr->email), addr->email);
}

This restricts printf to output at most the whole character array, but not more.

Upvotes: 4

haccks
haccks

Reputation: 106012

You can do the following, assuming that str1 and str2 are character arrays:

strncpy(str1, str2, sizeof(str1) - 1);
str1[sizeof(str1)-1] = '\0';

This will always set the last character to \0, no matter how long str2 is. But keep in mind that if str2 is bigger than str1, then the string will be truncated.

Upvotes: 2

Related Questions