inexplicable variadic bug with VS 2005?

[Henry Townsend]

I'm converting a piece of working C code from VS 2003 to VS 2005 and
have encountered a failure which just seems impossible to me. I've
scrutinized every line here and it's just not wrong. There's a
trimmed-down test case below and I'd be very grateful if someone could
point out my mistake.

Thanks in advance,
HT

BUILD SEQUENCE:

cl /Od /D "WIN32" /D "_DEBUG" /D "_CONSOLE" /MTd /W3 /nologo /c /Zi vsn.c

link vsn.obj /INCREMENTAL:no /NOLOGO /MANIFEST
/MANIFESTFILE:"vsn.exe.intermediate.manifest" /DEBUG /SUBSYSTEM:CONSOLE
/MACHINE:X86

CODE (vsn.c)

#include "io.h"
#include "stdio.h"
#include "stdarg.h"
#include "string.h"
#include "stdlib.h"

#define SEP ','

static char *
vsn_asprintf(const char *fmt, ...)
{
va_list ap;
char stkbuf[1024];
size_t bufsiz;
char *buf;
int len;

bufsiz = sizeof(stkbuf);
memset(stkbuf, 0, sizeof(stkbuf));

va_start(ap, fmt);

//DebugBreak();
len = _vsnprintf_s(stkbuf, bufsiz, _TRUNCATE, fmt, ap);

va_end(ap);

buf = (char *)malloc(len + sizeof(char));
strcpy_s(buf, sizeof(stkbuf), stkbuf);

return buf;
}

int
main(int argc, char * const *argv)
{

const char *hdr;

hdr = vsn_asprintf("%lu%ld%c%lu%ld%c",
0L, 0L, SEP, 0L, 0L, SEP);

_write(2, hdr, (unsigned)strlen(hdr));
_write(2, "\n", 1);
return 0;
}

[Henry Townsend]

BTW, I'm aware that there's some error checking missing and that the
return of malloc need not be cast. These are the result of breaking the
test case out of a much larger program.

HT

[Alex]

And what is the expected output, and what went wrong?

[Alex]

Ah, I narrowed it down to only having effect when you link against the
debug CRT, whether it's /MDd or /MTd it causes the crash. The debugger
mentions a heap corruption somewhere. Rather wierd.

[Alex]

Ah, nevermind, I found the bug in your code. It is indeed a heap
corruption -- you write way past your allocated buffer. You also have
a memory leak, but that's minor (you never free the buffer you
allocate).

The heap overrun is here:
//strcpy_s(buf, sizeof(stkbuf), stkbuf); // WRONG!
strcpy_s(buf, len+1, stkbuf);

You incorrectly tell strcpy_s() that the destination buffer is 1024
bytes long, when you only actually allocate (len+1) bytes.

[Henry Townsend]

Alex wrote:
> Ah, nevermind, I found the bug in your code. It is indeed a heap
> corruption -- you write way past your allocated buffer. You also have
> a memory leak, but that's minor (you never free the buffer you
> allocate).
>
> The heap overrun is here:
> //strcpy_s(buf, sizeof(stkbuf), stkbuf); // WRONG!
> strcpy_s(buf, len+1, stkbuf);
>
> You incorrectly tell strcpy_s() that the destination buffer is 1024
> bytes long, when you only actually allocate (len+1) bytes.

Thanks, but I don't think you're right on this. What you say would be
true for memcpy() but strcpy and friends assume a null-terminated
string. What MSDN says for the 2nd parameter is "Size of the destination
string buffer" and for the 3rd "Null-terminated source string buffer".
Thus, at least as I understand it, the 2nd param should be the entire
size of the buffer which is 1024 bytes, though only len+1 bytes will be
copied due to the null-termination. Here's what MSDN says for vsnprintf_s:

"Each of these functions takes a pointer to an argument list, then
formats and writes up to count characters of the given data to the
memory pointed to by buffer *and appends a terminating null*".

HT

[Alex]

I'm afraid that's irrelevant. If you carefully look at the docs for
strcpy_s, you'll find:
"The debug versions of these functions first fill the buffer with
0xFD."
(If you look inside a debugger, you will see this happening.)

This is a security precaution to help you find bugs in your code. You
told it the destination buffer is 1024 bytes, so it believes you and
tested your theory. Unfortunately, you only allocated 7 bytes, so you
overran it by 1017 bytes. The reason release build works is because it
doesn't perform this extra step.

So either allocate more space, or pass the correct amount to strcpy_s.

[Henry Townsend]

Alex wrote:
> I'm afraid that's irrelevant. If you carefully look at the docs for
> strcpy_s, you'll find:
> "The debug versions of these functions first fill the buffer with
> 0xFD."
> (If you look inside a debugger, you will see this happening.)
>
> This is a security precaution to help you find bugs in your code. You
> told it the destination buffer is 1024 bytes, so it believes you and
> tested your theory. Unfortunately, you only allocated 7 bytes, so you
> overran it by 1017 bytes. The reason release build works is because it
> doesn't perform this extra step.
>
> So either allocate more space, or pass the correct amount to strcpy_s.

Yes, you are right and I was wrong. I don't know how stared at this so
many times without seeing it but I did. Thanks for the clue.

HT