CODEWITHSUNDEEP
cdebuggingstack-overflow
coanor
coanor

Reputation: 3944

Why this stack smashing?

I got the following code(part of a log function):

/* set to 32 on purpose */
#define MAX_LOG_MSG_SZ 32

void log(const char *fmt, ...) {
    ....

    char msg[MAX_LOG_MSG_SZ] = {0};
    int nb_bytes = 0;

    /* get current time */
    time_t now = time(NULL);

    char time_buf[32] = {0};

    /* format time as `14 Jul 20:00:08`, and exactly 16 bytes */
    strftime(time_buf, sizeof(time_buf), "%d %b %H:%M:%S", localtime(&now));

    nb_bytes = snprintf(msg, sizeof(msg), "%s", time_buf);

    va_list ap;
    va_start(ap, fmt);
    vsnprintf(msg + nb_bytes, MAX_LOG_MSG_SZ, fmt, ap);
    va_end(ap);

    ....
}

The tricky things is when passing long parameters(make it longer than 32 bytes) and change time_buf to other value less than 32 (larger than 16, e.g., 31), these code will throw a stack smashing. After minutes debugging, I changed the vsnprintf calling line as

 vsnprintf(msg + nb_bytes, MAX_LOG_MSG_SZ - nb_bytes, fmt, ap);

and stack smashing gone, I think the problem fixed.

BUT: On time_buf[32](or other larger size), why the error calling

 vsnprintf(msg + nb_bytes, MAX_LOG_MSG_SZ, fmt, ap);

not throw a stack smashing? More exactly, why msg's stack smashing related to that unrelated stack (time_buf) space?

UPDATE: this is my uname -a output:

 Linux coanor 3.5.0-34-generic #55-Ubuntu SMP Thu Jun 6 20:20:19 UTC 2013 i686 i686 i686 GNU/Linux

Upvotes: 0

Views: 153

Answers (2)

bsd
bsd

Reputation: 2717

char time_buf[32] = {0};
/* format time as `14 Jul 20:00:08`, and exactly 16 bytes */
strftime(time_buf, sizeof(time_buf), "%d %b %H:%M:%S", localtime(&now));

nb_bytes = snprintf(msg, sizeof(msg), "%s", time_buf);

So effectively time_buf and msg contain the same data. snprintf returns the number of characters that would have been successfully written to msg not counting the null character.

vsnprintf(msg + nb_bytes, MAX_LOG_MSG_SZ, fmt, ap);

You are trying to write from the address given by msg+nb_bytes. You had 16 characters in msg. But you claim that you have MAX_LOG_MSG_SZ which is 32 characters. You are trying to write to end of the string. Perhaps fmt contains more than 15 characters.

vsnprintf(msg + nb_bytes, MAX_LOG_MSG_SZ - nb_bytes, fmt, ap);

This time you properly subtract the characters which have already been written to msg and give 16 characters to write to vsnprintf. It obeys and does not write beyond the end of the character array.

Upvotes: 1

junix
junix

Reputation: 3211

Using stack based buffers, you have to take care of not overrunning the allocated buffer. As you found out yourself, you introduced a possible buffer overflow using vsnprintf(msg + nb_bytes, MAX_LOG_MSG_SZ, fmt, ap);. This is, because you tell vsnprintf, there is more space available than there really is (because nb_bytes = snprintf(msg, sizeof(msg), "%s", time_buf); already wrote some bytes to the buffer).

Hence your fix, to pass MAX_LOG_MSG_SZ - nb_bytes instead of MAX_LOG_MSG_SIZE, is correct to avoid this effect.

It's also important to know, that snprintf and it's variants always return the number of Bytes that would have been written, regardless of the number of bytes actually written to the buffer.

EDIT: So in your case, you have to track the total length of the string during it's composition to make sure you don't overrun the total message buffer length.

Upvotes: 1

Related Questions