How can I check whether a parameter is a pointer to a stack variable?


A customer traced a bug in their program back to the fact that somebody passed a stack buffer to a function that operates asynchronously. Something like this:

// This function sends the buffer to the destination
// asynchronously. When the send completes, the completion
// handler is invoked.
HRESULT SendDataAsync(void *buffer, size_t bufferSize,
                      ITransferCompletion* completion);

class ColorSender : public ITransferCompletion
{
...
};

// Code in italics is wrong
HRESULT ColorSender::SendColor()
{
  COLORREF color = GetColorToSend();
  return SendDataAsync(&color, sizeof(color), this);
}

This bug led to the destination sometimes receiving corrupted data because it was a race between the stack variable being reused for something else and the data transport code copying the buffer into the transport channel. In this particular case, the customer fixed the problem by making the color local variable a member of the Color­Sender class, because the Color­Sender object itself must remain valid for the lifetime of the transfer seeing as it is the completion handler. (We're using the principle that code is mostly correct.)

The customer wanted to add some debugging code to the Send­Data­Async function so that it can assert that the buffer is not on the stack. The customer understands that this may be overly restrictive, and would not be appropriate for a function with a broad audience, but this function is used only within their application, so being extra-restrictive is probably okay.¹

On Windows 8 and higher, the customer can use the Get­Current­Thread­Stack­Limits function to get the bounds of the current thread's stack and use that to verify that the buffer pointer is not in that range. (Since this is just for debugging purposes, they can skip the test if running on older versions of Windows, and hope that their testing on Windows 8 will catch the problem.)

¹ If it turns out that the check is too restrictive, they could create two functions. The first is called Send­Possibly­Stack­Data­Async that does no validation, and the second is Send­Data­Async which does the initial validation that the buffer is not on the stack, and then calls Send­Possibly­Stack­Data­Async. Most callers use Send­Data­Async, but if you're in a special case where you are sending stack data and you know that you won't return from the function until the completion occurs, then you can use Send­Possibly­Stack­Data­Async. The team could have a rule that equires all uses of Send­Possibly­Stack­Data­Async to undergo a special review.

Comments (34)
  1. Mason Wheeler says:

    As one of my coworkers once said (actually before he died, not after,) Dennis Ritchie’s true legacy to the world is the buffer overrun.

    1. D Taylor 84 says:

      Or, perhaps more appropriate to this story, the dangling pointer…

    2. McBucket says:

      Struck by lightning, eh?

      (not meaning any disrespect to your colleague, of course)

      1. exchange development blog team says:

        That’s ‘struct by_lightning’ to you, your version won’t compile.

    3. Drak says:

      For a second I was wondering how your coworker would have been able to say anything after he had died, but then I figured out what you meant.

  2. PaulTDessci says:

    I’ve always felt that the C/C++ Standard Library should provide the ability to check whether a pointer or object is on the stack, heap, or static storage. These memory categories are acknowledged in the language spec and it is not as if it would restrict implementations, at least AFAIK. Of course, such facilities could be abused but that could be said of C++ programming in general. Perhaps the promised reflection facilities due to be added to the language real soon now will help.

    1. zboot says:

      And how exactly would the compiler know the difference between the stack and heap memory?

      Once you answer that question, then explain how that doesn’t restrict an implementation.

  3. Ken Hagan says:

    No need to be harsh on DMR. This is a design problem, not a language problem.

    Change the semantics of SendDataAsync() so that it takes ownership of the buffer and deletes it afterwards. That makes it almost impossible to pass a stack buffer in without it crashing the very first time you run the code.

    What’s that you say? The client wants to retain the buffer so that they can re-use it themselves? Sounds like I’ve just saved you from *another* race condition, by forcing you to make a copy.

    1. Ben Voigt (Visual Studio and Development Technologies MVP with C++ focus) says:

      Yes, you can use Java instead of learning to write C++ correctly. But you also find that correctly-written C++ is significantly faster than the language-with-training-wheels equivalent.

      1. In situations where performance is critical, write in C. Otherwise, write in the language than enables you to write correct code easiest. :-)

        1. Evan says:

          > In situations where performance is critical, write in C.

          This is a bit trolly, but I at least believe to an extent… but I’d change that to “if performance is more critical than correctness or security, write in C.”

          1. Nick says:

            I promise you I can write very fast code that is wrong. exit(0) comes to mind.

      2. exchange development blog team says:

        And then you find all the holes are in the JVM instead of your code. With C you can at least get it right (think MISRA and others). With Java you can write the best code in the world but you’re still at the mercy of the JVM. MISRA can’t even deal with C++ yet, let alone Java.

        1. exchange development blog team says:

          Argh, DO-178B, not MISRA for the C++. Knew that was wrong as soon as I’d clicked “Post”.

        2. voo says:

          You seem to be misunderstanding what MISRA (or DO-178B) actually are. They are guidelines for developing specific systems in specific languages.

          It’s not that it’d be particularly hard to develop a similar standard for Java (and quite a few things apply to any language), but there’s just less of a demand for it – particularly since many of the issues that MISRA deals with are non issues in Java to begin with (make sure that your linker supports identifiers with more than 6 characters?)

        3. Sure, until a vulnerability is discovered in the standard library. Or MFC, ATL, or one of the dozens of libraries your project is dependent on.

          At the end of the day, you’re always at the mercy of your dependencies, regardless of the language.

      3. Voo says:

        The fun thing about C++ ideologists is that every time there’s another problem due to C++’s design they always tell you that that’s all due to stupid people not knowing how to write “correct” c++.

        Sadly they’re never able to provide a link to a large open source project that is written in “correct c++” and shows that it’s possible to actually write a large c++ program without buffer overflows, use after free and all the other fun things.

        Mind you, that doesn’t mean that it’s impossible to design a reasonable safe language without GC as rust shows.

        1. zboot says:

          Maybe people are busy writing non open source large scale “correct” c++ programs, not vetting every single open source project so they could one they prove a specious point.

          1. Voo says:

            So you’re saying “correct c++” is such a rarity that there’s just no open source project out there that manages it?

            Well OK, so what are the closed source ones? As long as it’s one with a public bug tracker we can probably still get a good idea about it.

          2. santosh says:

            Chromium?

        2. David Haim says:

          Well let’s see.
          how about Windows, Mach and Linux as Operating systems? how about the JVM, the CLR, WebKit and V8 ? HHVM? FireFox and Chrome? shall I continue?

          1. He’s not saying impossible, just that it’s really hard to do. All of those projects were created by really smart, well-paid people and even then they still have suffered (and in many cases continue to suffer) from pretty well-understood bugs like buffer overruns, heap spray attacks, memory leaks, etc.

          2. voo says:

            Umm, every single one of those projects had thousands of bugs due to out of bounds indizes, use after free, memory leaks and all the other fun C++ bugs.

            If anything all of those are great examples how even really clever and good programmers cannot create reliable software in C++. Yes all those people who say that in theory you could avoid all those bugs are right – in theory. In practice there exists not a single large C++ project that has ever managed to write “correct C++”.

          3. All software has bugs. While in theory a single out-of-bounds memory write, use-after-free, etc. is Undefined Behavior and can erase your hard drive or make demons fly out your nose, in practice things like that that don’t *usually* happen.

            The choice of language will impact the nature of the bugs written, so a C++ project will be more likely segfault on a bug, while a Java project will be more likely to throw an ArrayIndexOutOfBoundsException on a bug, and a PHP project will be more likely to have some type coercion bug and spew nonsense into your HTML output. Good programmers will write solid code with fewer bugs per unit of code regardless of the language, and bad programmers will write more bugs per unit code.

        3. Joshua says:

          Voo, you play the troll. I have seen it. I’m not kidding; a major open source C++ project with version number > 1.20 that as of 1.0 has had zero buffer overflow security exploits. It’s not even hard because there is a string class.

          1. voo says:

            And the name of that project would be? I mean I’ve seen so, so many people claim the existence of such projects and how it wouldn’t be hard if you just followed some simple rules, but somehow nobody ever actually names an example.

            I mean I’d really like to see their style guidelines, static analysis tools and code review rules if they manage to keep more than a handful people over several years from never introducing a single bug of the kind.

  4. Myria says:

    If you want to implement this on older versions of Windows than 8, the values you want are in reinterpret_cast(NtCurrentTeb()). The StackBase and StackLimit members are what you should check against. Note that StackLimit is the *lower* value, because stacks grow downward on Windows platforms. (The Itanium complication to that statement isn’t relevant here.)

    1. Myria says:

      Really? This is parsed as HTML?

      reinterpret_cast<NT_TIB *>(NtCurrentTeb())

  5. mihailik says:

    Stack versus heap is a red herring, the root cause is design flaw of the API.

    If the caller has no control over how long the buffer is in use, then heap-allocated buffer has the same vulnerability: whilst the function is doing its async processing the caller may change the contents of the buffer, or reclaim/reuse the memory altogether.

    Checking for stack allocation is not restrictive enough: you merely get LESS corruption. Arguably, rare and hard-to-reproduce corruption on the heap will be even costlier to track and fix.

    So, good luck! ;-)

    1. Ivan K says:

      Agreed. If the caller has no control of how long the buffer is in use, then that would be a problem.
      The “buffer” would either need to be some sort of reference-counted object like how IAdviseSink works, or there would need to be some sort of “unsubscribe” type of method that took a “cookie” provided by some sort of “subscribe” method, with the API providing a guarantee that the buffer would not be used after the “unsubscribe” method returned, so it could be safely freed after the call to “unsubscribe” (making threading issues the user of the API’s responsibility to handle). Or something else to that effect that makes the whole thing work. “Assuming that code is mostly correct” and that the blog article is edited for brevity makes me confident that this is indeed the case…

  6. kantos says:

    To me this is an issue of ownership, something the current C++11 and up semantics make abundantly clear:

    * Raw pointer is an unowned optional
    * Owned pointers are always smart pointers
    * Sinks should take in a smart pointer
    * non-optional pointers should be references

    Sadly this doesn’t work with WIN32 which is a C api… but that’s what a wrapper is for.

  7. Killer{R} says:

    This check could be done with static analysis. There’re already different arguments hints for analyzer, like __in, __out, __in_opt. So adding something like __in_no_stack/__out_no_stack could be helpful for this, since static analyzer may track argument’s origin.

  8. Stephen Steel says:

    The concern with stack vs. heap allocation is completely irrelevant. The problem is that the code violates the implicit lifetime contract that the SendDataAsync() function requires of the buffer passed as an argument: namely, that the it live until the asynchronous transfer completes (or is cancelled). Moving the buffer to a scope with the appropriate lifetime, the ColorSender object, solves the problem. It would be the appropriate fix even if the ColorSender object is itself allocated on the stack.

    This is a case where a safe, easy to use interface contracts and performance requirements conflict: a safe, easy to use interface contract would have the SendDataAsync() function either make a copy of the buffer, or use something like a std::shared_ptr to ensure the buffer data remains available after SendDataAsync() returns. However, high performance means avoiding the extra copying or smart pointer overhead.

    The C++ Core Guidelines help to address some of these concerns by making ownership and lifetime more explicit.

  9. Cesar says:

    Another commenter already mentioned the Rust language, but to go into more detail: in the Rust language, the prototype for the SendDataAsync function can specify that `buffer` must have a lifetime at least as long as `completion`, and the compiler verifies that (it’s a compilation error otherwise).

Comments are closed.

Skip to main content