Dangerous Documentation Part 2–Printing Strings

The printf family of functions encompasses a vast range of possibilities. Printing to a file or to memory, with or without locale specified, ‘n’ variants, ‘v’ variants, ‘c’ variants, ‘_s’ variants, wide-character variants – it’s an exponential explosion of complexity.

Today we cover just one function – wsprintf – and find that its documentation is contradictory, and dangerously wrong.


wsprintf

A quick office poll showed most programmers think that wsprintf is the wide-character version of sprintf. That is incorrect. The function they are thinking of is swprintf. So what the heck is wsprintf?

The documentation for wsprintf (as of October 2011) fails to explain this confusing name, but as far as I can tell the ‘w’ indicates that this is a Windows function. As is common with Windows text functions there are ANSI and UNICODE versions of this function. That means that wsprintf is actually a macro that expands either to wsprintfA or wsprintfW. So far so weird.

And now, a few quotes from the documentation:

The function appends a terminating null character to the characters it writes

The string returned in lpOut is not guaranteed to be NULL-terminated.

Pointer to a buffer to receive the formatted output. The maximum size of the buffer is 1024 bytes.

Also, avoid the %s format — it can lead to a buffer overrun

Where to begin…

The two most important security considerations when dealing with string buffers are avoiding buffer overruns, and guaranteeing null-termination. The documentation for this function mangles both of these.

Null termination

The documentation says that this function null-terminates the buffer, and it also says that it might not. I have tested this function and I have been unable to find a case where it does not null-terminate the buffer. If I pass it bad source pointers then it crashes, so it does not appear to have the vulnerability that lstrcpyn has.

My best guess is that the warning about NULL termination not being guaranteed is simply an error. Or maybe versions of wsprintf on older operating systems (I only tested on Windows 7) don’t always null-terminate the buffer. The documentation fails to elaborate, which leaves the reader with no way to tell whether this potentially important warning applies to them or not.

Buffer size

wsprintf is a weird function in that it does not take a buffer size parameter, but it has a documented maximum number of characters that it will print. This means that wsprintf is dangerous and can lead to buffer overruns (with or without using %s) if your buffer is smaller than this maximum, and wsprintf is safe if your buffer is as large as this maximum.

Based on the statement “The maximum size of the buffer is 1024 bytes.” in the documentation it seems clear that as long as your buffer is 1024 bytes long you are fine.

Remarkably this simple and critical sentence contains two bugs.

The first bug is that the buffer size that wsprintf assumes is actually measured in characters, not bytes.

The second bug is that the correct maximum size is 1025, not 1024. Yes, the documentation has an off-by-one error. Or the code does. But since the code has shipped to a billion customers it is grandfathered in, so the documentation is, by definition, wrong.

The actual behavior of wsprintf can be measured using the example code below. It fills a 2,000 character buffer with a long string, and then prints it to another 2,000 character buffer using wsprintfW. Because of the hardcoded buffer length limitation in wsprintfW the string is truncated to 1024 WCHARs – plus a terminating zero – for a total of 1025 WCHARs, or 2050 bytes.

WCHAR wsrc[2000];
for (int i = 0; i < ARRAYSIZE(wsrc) – 1; ++i)
    wsrc[i] = ‘a’ + (i % 26);
wsrc[ARRAYSIZE(wsrc) – 1] = 0;

WCHAR wdst[2000];
memset(wdst, 0, sizeof(wdst));
wsprintfW(wdst, L”%s”, wsrc);
size_t wLen = wcslen(wdst);
printf(“wsprintfW of %u WCHARs gives a length of %u WCHARs\n”,
(unsigned)ARRAYSIZE(wsrc) – 1, (unsigned)wLen);

This code prints:

wsprintfW of 1999 WCHARs gives a length of 1024 WCHARs

The code doesn’t lie.  Unless I’ve made some grievous error, wsprintfW assumes that you will pass it a 1025 character buffer.

This strikes me as being kind of important. Even if people correctly guess that by ‘bytes’ the documentation means ‘characters’, the off-by-one problem remains. If you pass wsprintf a 1024 character buffer then you are at risk of a one-character buffer overrun. That’s not the worst type of buffer overrun, but it’s sloppy and there have been cases where a one-byte overrun with zero was sufficient to pwn a machine.

In many uses of wsprintf developers have probably made sure that their use of wsprintf is safe by calculating the maximum length of the string that they could print. But calculations like that are difficult to get right 100% of the time, which is why sprintf is deprecated. If you pass wsprintf a buffer that is smaller than 1,025 characters then it is just as dangerous as sprintf.

Code Analysis

Microsoft is still using this function (my system32 directory lists 51 imports of wsprintfA and wsprintfW) and it seems quite likely that they are using it incorrectly. Normally code analysis would come to the rescue and detect unsafe usage, but this function is not annotated, so code analysis has no idea how much data these functions might write. That’s odd since I thought Microsoft’s policy was to require annotations on all buffer filling functions. Annotations like those shown below, added to winuser.h, would allow code analysis to detect dangerous uses of wsprintf:

WINUSERAPI
int
WINAPIV
wsprintfW(
_Out_z_cap_c_(1025) LPWSTR,
_Printf_format_string_ LPCWSTR,
…);

Summary

The documentation for wsprintf contains an inappropriately alarmist warning that %s cannot be used safely, contradictory information about whether the output buffer will be null terminated, and incorrect information about how many characters may be written. Additionally, the example code that it links to no longer uses wsprintf so there are no best-practices to follow.

While poking around for other information about wsprintf I found this very old article, which just made me very grateful we don’t have to deal with those issues anymore.

Getting documentation clear and correct is important, and wsprintf needs work in both areas.

wsprintf documentation: F

About brucedawson

I'm a programmer, working for Google, focusing on optimization and reliability. Nothing's more fun than making code run 10x faster. Unless it's eliminating large numbers of bugs. I also unicycle. And play (ice) hockey. And juggle.
This entry was posted in Code Reliability, Documentation, Programming, Visual Studio. Bookmark the permalink.

5 Responses to Dangerous Documentation Part 2–Printing Strings

  1. Riley L says:

    One of the biggest things to take away from this in my opinion is simply being smart about naming a function. It doesn’t need to go through a bureaucratic process or anything but taking a couple seconds to realize that it could end up being very confusing to others is the first step.

  2. RichR says:

    Thanks for bringing some clarity to the obscure documentation. In trying to write safely, the wsprintf documentation and its recommended replacements have driven me to distraction and this post has been reassuring.

    • brucedawson says:

      In many cases the safer choice is to avoid wsprintf, and use sprintf_s or _snprintf_s instead. With its ability to infer buffer sizes through template magic it makes the buffer size problem disappear:

      char buffer[100];
      _snprintf_s( buffer, _TRUNCATE, “This will truncate rather than overflow – %s\n”, pLongString );
      sprintf_s( buffer, “This will abort rather than overflow – %s\n”, pLongString );

  3. frymaster says:

    quick nitpick: “deign” means “does something that the person thinks is beneath their dignity”, it doesn’t mean “doesn’t do something”

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s