What is wrong with this buffer overflow solution in c?

Question

I was just marked down on coursework for this incorrect solution to a buffer overflow in c but was not provided feedback on how it was wrong. Could somebody let me know what the problem is? Thank you.

The question stated to provide a solution in case a longer string than 16 was passed in to this function:

void function(char *str)
{
    char buffer[16];
    strcpy(buffer, str);
}

And here is my solution

void function(char *str)
{
    size_t str_length = strlen(str);

    char buffer[str_length];
    strcpy(buffer, str);
}

Thanks

Your solution also relies on variable length arrays, which were not available until C99. It's possible that the instructor is not aware that they are allowed now. — Bill Lynch
– Bill Lynch, Commented May 19, 2015 at 0:28

JS1 · Accepted Answer · 2015-05-19 03:39:26Z

11

You need to account for the null character terminating the string:

char buffer[str_length + 1];

Void_ptr points out that the above is not enough. So to be more robust:

void function(char *str)
{
    size_t str_length = strlen(str);
    char *buffer = malloc(str_length + 1);
    if (buffer == NULL)
        return;
    memcpy(buffer, str, str_length+1); // Thanks chux
    // Do something with with buffer...
    free(buffer);
}

Or maybe the professor was simply looking for this:

void function(char *str)
{
    char buffer[16];
    strncpy(buffer, str, sizeof(buffer));
    buffer[sizeof(buffer)-1] = '\0';
}

edited May 19, 2015 at 3:39

answered May 19, 2015 at 0:26

JS1

4,7671 gold badge17 silver badges20 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

void_ptr Over a year ago

Still looks kinda unsafe. 1) buffer is allocated on the stack - what if you get stack overflow? 2) There's no upper limit on str_length - what if there's no terminating 0 for gigabytes and gigabytes of memory space?

chux Over a year ago

Code paid the price to calculate the length of str. strcpy() effectively does that again. Instead of strcpy(buffer, str);, consider the more efficient memcpy(buffer, str, str_length+1).

cfz42 · Accepted Answer · 2015-05-19 01:06:48Z

1

It depends on what you are allowed to use but I guess the safest way would be to use strdup and check if it has returned NULL.

void function(char *str)
{
    char *buffer;

    buffer = strdup(str);
    if (!buffer)
        exit(EXIT_FAILURE);
    free(buffer);
    /* free buffer or keep track of it or you'll end up with memory leaks */
}

In the case where you're not allowed to dynamically allocate memory, the use of strncpy is still a more secure alternative to strcpy.

void function(char *str)
{
    size_t str_length = strlen(str);
    char buffer[str_length + 1]; /* /!\ This is C99 */

    strncpy(buffer, str, str_length);
    buffer[str_length] = '\0';
}

answered May 19, 2015 at 1:06

cfz42

3842 silver badges15 bronze badges

1 Comment

chux Over a year ago

1) strncpy(buffer, str, str_length); buffer[str_length] = '\0'; serves no additional security. Just use memcpy(buffer, str, str_length+1); 2) There is no limit to the size of buffer[] - now that is a stack smashing security issue.

Collectives™ on Stack Overflow

What is wrong with this buffer overflow solution in c?

2 Answers 2

2 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related