CODEWITHSUNDEEP
cbuffer-overflow
CraygL
CraygL

Reputation: 67

What is wrong with this buffer overflow solution in c?

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

Upvotes: 1

Views: 580

Answers (2)

JS1
JS1

Reputation: 4767

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';
}

Upvotes: 11

cfz42
cfz42

Reputation: 384

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';
}

Upvotes: 1

Related Questions