Even if a function doesn’t do anything, you still have to call it if the documentation says so, because it might do something tomorrow


If the documentation says that you have to call a function, then you have to call it. It may be that the function doesn't do anything, but that doesn't prevent it from doing something in the future.

Today's example is the function GetEnvironmentStrings, which returns you all the environment variables of the current process in a single block, which you can then study at your leisure. When you're finished, you're supposed to call FreeEnvironmentStrings. That's what the documentation says, and if you did that, then you're in good shape.

However, some people noticed that on Windows NT 4, the Unicode version of the FreeEnvironmentStrings function didn't do anything. In other words, the Unicode environment block didn't need to be freed. When you called GetEnvironmentStrings, the kernel just returned you a raw pointer to the real live environment strings (which, since this is Windows NT, are kept in Unicode internally). Since nothing was allocated, there was nothing to free.

The problem with this technique was that if somebody called SetEnvironmentVariable in the meantime, the environment block changed out from under the caller of GetEnvironmentStrings.

Oops.

To fix this, the GetEnvironmentStrings function was changed to return a copy of the environment block even if you call the Unicode version. The corresponding Unicode FreeEnvironmentStrings function frees that environment copy.

Programs that followed the specification and called FreeEnvironmentStrings (even though it didn't do anything up until now) were in good shape. Their call to FreeEnvironmentStrings now frees the memory, and all is right with the world.

Programs that coded to the implementation rather than the specification are now in a world of hurt. If they simply skipped the "useless" call to FreeEnvironmentStrings, they will now find themselves leaking memory. On the other hand, if they gave lip service to FreeEnvironmentStrings by calling it, but using the memory anyway, they will find themselves accessing invalid heap memory, and all sorts of havoc can ensue.

There's sometimes a reason for the rules that seem stupid at first glance. ("Call this function that doesn't do anything.") Changes to the implementation may make them less stupid in the future.

(Credit goes to my colleague Neill Clift for providing the information that led to today's article.)

Comments (38)
  1. Karellen says:

    How would someone know that the call to FreeEnvironmentStrings() did nothing? Do some people regularly disassemble API functions to see if it’s OK to not call them or something?

  2. Q says:

    "To fix this, the GetEnvironmentStrings function was changed to return a copy of the environment block even if you call the Unicode version."

    When did that happen? It appears XP SP3 (+ all updates) still returns a direct pointer:

    kernel32!GetEnvironmentStringsW:

    7c812f98 64a118000000     mov     eax,fs:[00000018]

    7c812f9e 8b4030           mov     eax,[eax+0x30]

    7c812fa1 8b4010           mov     eax,[eax+0x10]

    7c812fa4 8b4048           mov     eax,[eax+0x48]

    7c812fa7 c3               ret

    kernel32!FreeEnvironmentStringsW:

    7c814b77 33c0             xor     eax,eax

    7c814b79 40               inc     eax

    7c814b7a c20400           ret     0x4

  3. Nathan_works says:

    Ah irony. Q indirectly answers Karellen’s question.

    I still have bad memories of our architect doing similar things. The "who cares what MSDN says, their code doesn’t do anything, so don’t call it"..

  4. t3hm4x says:

    Q:

    32bit Vista the changes Raymond mentions take place as well as the 64bit editions as well.

  5. Anonymous Coward says:

    Sorry, but if we’d have to read the MSDN page for every single method of every single class we have ever used (citing .NET Framework’s 10.000+ classes as an example), we would not have finished writing any reasonably complex piece of software. That’s why people are relying on conventions and recurring patterns in the blackbox code they use. Changing API implementations – after the fact – might seem correct for the engineer who designed the API (and knows his/her "own" MSDN page by heart), but not for the rest of us.

  6. Leo Davidson says:

    Anonymous Coward, please stop writing software if you think it’s better to guess about what APIs do rather than read their contracts. Your software must only work by chance.

    It’s not like it takes long to push F1 and read about the function (at least if you’re writing .Net and F1 still takes you to the function for your language/framework and not Windows CE, MFC or .Net).

    I’d argue that it would take longer to experiment with the API to determine its behaviour than it would to simply skim the documentation.

    Whenever the OS gives you a piece of data, especially when — as is the case here — we are talking unmanaged C/C++ and not .Net, the first thing you should be asking is "who owns this memory/object and how is it freed? The second thing should be "what are the error return values and/or behaviours. You don’t get any of that by guessing.

    Back to the root post: This is why I still call GlobalLock and GlobalUnlock even though they "don’t do anything anymore".

  7. Matt Craighead says:

    The sentiment is great, but this seems like one of those cases where you’re simply doomed if you want to change the API’s implementation.  For all practical purposes, this is a compatibility break.

    It’s hard enough to make internal API changes that affect 100 other developers on your own team.

    Sometimes I’ve been tempted to have an API implementation walk up the call stack and rewrite (using self-modifying code) the buggy calling code.  Never actually done that, though.

  8. @Leo says:

    Do you still need to use Global or Local Memory functions?  I suppose there are a few system APIs which require memory allocated from them, but I’d stick with the CRT or the Heap* APIs for any other uses.

  9. SuperKoko says:

    "Back to the root post: This is why I still call GlobalLock and GlobalUnlock even though they "don’t do anything anymore"."

    Don’t GlobalLock and GlobalUnlock do something for GMEM_MOVEABLE memory?

    I go further. I call UnlockResource, even though it’s clearly marked as obsolete and not doing anything, and probably implemented as a macro doing nothing. I just feel that there’s something wrong with locking something but not unlocking it. I’m not sure that it’s available in latest Win32 SDK headers.

    I’m curious. Do FreeEnvironmentStrings do anything in Windows 95/98/Me? Win32 != Windows NT/2000/XP! Assuming that Windows is necessarily based on the NT kernel is stupid!

  10. "For all practical purposes, this is a compatibility break." – As Leo Davidson says, for <i>every</i> bit of data in C(++), you must always know who is responsible for freeing it, it is part of the language itself, and there is no way to represent this in the language itself so you have no choice. If you start with the assumption that people won’t do this… well, you might as well not write the code at all, because they have to.

    You just can’t build APIs built on the assumption that the API users won’t know the language the API is in. (This isn’t an absolute, it’s good to think about common mistakes, how to catch them and mitigate them, but at fundamental level you have to assume your API users know how the language works, because without that your API doesn’t matter, those programmers are screwed no matter what.)

  11. Tony Morris says:

    You guys, who are married to the degenerate imperative programming mindset, have got it all backwards.

  12. Poochner says:

    @Jeremy: I disagree.  It’s quite possible and preferable for an API to be written in a language agnostic way.  That doesn’t mean you ignore common sources of programming errors, because they’re common to many programming languages.  To twist a phrase, a lousy programmer can leak memory in any language.  It’s just harder in some than others.  This is especially true in systems such as Win32 that return some resource that need to be freed at a later time, and can’t necessarily be just garbage collected.  File handles / descriptors / ports?  Something like that exists almost in almost every OS and if it’s just a random small number, well, there ya go.

  13. Eric TF Bat says:

    I must be a real old fart, because I saw this heading and thought immediately of FindFirst, FindNext and FindClose.  In Win 3.1, FindClose did nothing, but you were advised to call it anyway; sure enough, in Win95 FindClose was a dispose, and people who did the Right Thing [TM] were rewarded with fewer memory leaks.

    [Oh, you mean something like this. -Raymond]

    There are a lot of contracts in programming.  The one that DOESN’T exist is the one that says “This is going to be easy, so you don’t need to learn anything”.  Programmers who think they can coast on gut feeling and googled example code are bad programmers; no doubt about it.

  14. Cale Gibbard says:

    Why not instead design the API such that the programmer cannot fail to use it correctly and still have their program compile?

    I think this mindset of requiring the programmer to follow instructions rather than enforcing the rules in the language is primarily a property of the inexpressiveness of the languages under consideration.

    In this case for instance, in a slightly richer language, you could have a function WithEnvironmentStrings which takes a function that takes the environment strings to a procedure to be performed with them, and does whatever allocation and deallocation is required with regard to the table, with the procedure performed in between. You can do this in C too, but your users will hate you, because C has no anonymous functions or procedures.

    I think this is what Tony Morris was getting at with his remark that the imperative mindset has things all backwards. It’s too hard to express the constraints on how APIs are to be used in your language, so you pass the responsibility off to your users, and then get upset when they don’t follow your rules. :)

    Instead, it’s much nicer to handle issues like this using techniques such as garbage collection in the common case of things like memory, or continuation passing style allocation (like the WithEnvironmentStrings example I mentioned above) in more complex situations, but of course you need a language which provides you enough luxuries to express it.

  15. Nicholas says:

    @Anonymous Coward

    Apples and oranges!  If we are writing in this blog then we are writing about the Win32 API, which is not .NET, so don’t compare the two.

    The collection of API calls, while not small, is manageable.  Let me rephrase, the collection of API calls *that matter* is manageable.  There is no reason to scoff at the MSDN and go on your own based on disassembly listings of Windows’ DLL files.

    Are these errors in the MSDN?  Sure, it happens.  However, and I cannot speak for .NET development, I have always been satisfied with the level of detail the MSDN provides for *Win32 API* functions.  I have never needed to disassemble a Win32 function to figure out what is going on.

  16. Matt Craighead says:

    "you have to assume your API users know how the language works, because without that your API doesn’t matter, those programmers are screwed no matter what"

    Until you get blamed because your upgrade "caused" their program to stop working, at which point it is automatically your fault.

    I have a lot of sympathy for Raymond’s arguments in favor of compatibility.  It can be taken too far, though.

    In this case, I would have suggested that the original API should *not* have had a Free function, and that rather than changing the existing API, we’d have to create a new API GetEnvironmentStrings2() (or whatever you want to call it).  But then we get into the mess that this was only an issue with the Unicode version of the function, and… well… yuck.  (And as for Windows’s Unicode support strategy, which makes it painful to write code that handles Unicode and is portable across Windows/Linux/Mac, that’s a topic for another day… oh, if only you could pass UTF-8 strings to the "A" functions rather than them using the current code page…)

  17. Drak says:

    I think Cale means ‘in hindsight, it would have been better that …’ and it’s a good idea for the future.

  18. Miral says:

    The simple fact is that the documented contract has always required people to call the FreeEnvironmentStrings function afterwards.  If application vendors decided not to do so (whether because they thought they knew better or because they didn’t bother to read the documentation), then they deserve to get taken out behind the woodshed and thrashed.

    Having said that, as a user, if I upgrade to Vista and it makes an application I use frequently not work any more, then I am going to blame it on Vista, not the application.  Even if it really is the app’s fault :)  ("It worked perfectly *before* I installed Vista…")

    You just can’t win.

  19. SuperKoko says:

    "but of course you need a language which provides you enough luxuries to express it."

    You mean: Using a .NET language such as C#?

    You’re free to do so, as you’re free to use the good old simple Win32 API with a good old simple standard portable language such as C.

    One solution to prevent API contract violations, for older API, such as Win32, is to validate every input, even in release versions, since bad programmers don’t use debug tools. For system resources leaks, it cannot do more than logging the system leak to a clearly visible and accessible file, through a per-process counter for every type of resources. It has performances implications, though.

  20. Ens says:

    You want Windows programming to *require* the use of a newer and entirely distinct programming language which is not compatible with any known standard?

  21. wtroost says:

    In my experience documentation is only used for blaming other people.  This article is no exception. :)

  22. _ says:

    of course it is also incredible that the api was designed that way at first. system architects should be able to forsee the future problems of a no-op function. unfortunately all kinds of problems like this can arise in unmanaged code. they are much less likely in managed code.

  23. GregM says:

    "of course it is also incredible that the api was designed that way at first."

    Who said that the API was designed that way at first?

    "However, some people noticed that on Windows NT 4, the Unicode version of the FreeEnvironmentStrings function didn’t do anything."

    Notice that this says "on Windows NT 4" and "the Unicode version", not "since the function was first introduced, all versions of this function did nothing".

  24. Duke of New York says:

    If you program in a native language, you need to know what memory is. No excuses! The sad reality is that there are many native code programmers who don’t.

  25. Aaron says:

    Paraphrasing Anonymous, Matt, Cale, et al:

    "Why can’t you just design a telepathic and clairvoyant API that automatically fixes all my boneheaded bugs for me?"

    What surprises me isn’t that some people have a habit of writing poor code.  What surprises me is that they can be so entrenched in that habit that they will loudly proclaim that it’s really somebody else’s problem.

  26. M1EK says:

    As a sometimes systems-programmer myself, I’m dismayed that it took until just a couple of comments ago before somebody pointed out that it was ALSO a dumb thing to stick in a do-nothing function call and then assume people would call it by contract.

    Uh, did that REALLY seem like a good idea to anybody? By Windows NT 4, had we not all figured out a long time ago that many application developers are not going to do things exactly the way you tell them?

    This is MS’s error first; the app developer’s second. Neither one is right, but MS was more wrong.

    [Okay, you have a time machine. What do you do? Do you have the cleanup function do some pointless busy work just so it does “something”? -Raymond]
  27. APIs may be language-agnostic, but an API in a given language are automatically bound to the language you are using.

    I’ve seen Python and Perl wrappers of very complicated C(++) APIs that do indeed take away the problems of leaking (if we assume you trust the Perl/Python GC). But that doesn’t do anything to change the fact that if you use the native C(++) API, you’ve got to worry about such things.

    Demanding a C(++) API that automatically handles these things and makes the programmer not have to think is demanding something other than a C(++) API. Which in the abstract is OK; I’d program .Net before C(++) Win32 in a heartbeat. But that doesn’t change a single thing about Raymond’s post, which already starts with the assumption that we’re in C(++). And if you’re programming in C(++) without a constant awareness of memory issues, you’ve *already lost*, long before Microsoft changed an API in a way they told you about in advance. Lossage that I’ve seen more times than I can count, so it’s hardly theoretical.

  28. _ says:

    [Okay, you have a time machine. What do you do? Do you have the cleanup function do some pointless busy work just so it does "something"? -Raymond]

    no, you always create a copy for the application. what if the app decides to use the returned buffer as some kind of working memory an does string operations in it? this is not the least unlikely. future calls of the Get-api will then return the modified result.

  29. Duke of New York says:

    "what if the app decides to use the returned buffer as some kind of working memory an does string operations in it?"

    Then the app is ignoring the part of the documentation that says:

    "Treat this memory as read-only"

    Aside from that, the end-to-end principle applies. If you need a copy for applications-specific reasons, make a copy. It’s not the operating system’s job to write your application for you.

  30. _ says:

    "Then the app is ignoring the part of the documentation that says"

    this is constantly happening. after all, this blog is partially about the phenomenon of bad code. as a systems designer you cannot ignore this. i even have to think about it when coworkers are using my code!

    "If you need a copy for applications-specific reasons, make a copy. It’s not the operating system’s job to write your application for you."

    of course it is! the operating system provides services to you in order to alleviate the need for implementing them yourself. the reason for choosing a development platform is that one platform might reduce your work more than the other.

  31. Duke of New York says:

    The name of the API is "GetEnvironmentStrings." Its job, believe it or not, is to get environment strings and then let the application move on– not to anticipate whether you need a copy, how many, for how long, from which allocators, etc., etc. All of those are things the application can manage for itself, using other APIs, or no APIs.

  32. _ says:

    what would you have done at the time you created your first win32 window? i believe that you are now capable of using win32 correctly but only because you have gone through every misery unmanaged code has to offer at least once ;-) unfortunately many programmers are, after 10 years of experience, still at the entry level.

    an api can be designed to help you and it can be designed without consciousness of usability. GetEnvironmentStrings one is of the latter (although the usability problem is only minor. there are other examples as well).

  33. SuperKoko says:

    [Okay, you have a time machine. What do you do? Do you have the cleanup function do some pointless busy work just so it does "something"? -Raymond]

    I don’t think the implementation is flawed, but if I had a time machine I might take a very defensive approach: Counting resource references. Incrementing a process-wide counter when GetEnvironmentStrings is called and decreasing it when FreeEnvironmentStrings is called. If the count, when a process terminates or exit, is non-zero, this event would be logged.

    However, its limit are obvious:

    1) It doesn’t prevent application from calling FreeEnvironmentStrings on the wrong pointer.

    Adding testing logic is possible, but would necessarily GetEnvironmentStrings to copy the string.

    (An instance ordinal would be added in the bytes preceding the string… People would obviously mess with it…)

    2) Programmers would ignore the logs.

    3) It is making the release version of Windows look like a debug version. Just because bad programmers don’t use debugging tools, end users would suffer from performances of debugging tools.

    @M1EK: So, are you telling us that the average programmer doesn’t read the manual but disassemble the Windows code? I cannot believe that!

    I would rather expect the bad programmer not to care about resource leaks as far as the memory leaks don’t make his program crash in an "out of memory" condition afer 10 minutes, which is not likely to happen with little-used resources such as environment blocks and kernel handles.

    "what would you have done at the time you created your first win32 window?"

    I remember very well. I carefully read the SDK documentation and took an extremely defensive approach everywhere I wasn’t sure… Of course, as I usually do, I freed resources in the reverse order of their allocation. Later, as I better knew the documentation, I relaxed my programming technique.

  34. fdiv says:

    I never disassemble the Windows code to see what an API does and not just because the EULA forbids it. Most of the time either Raymond explains it, or I look at what Wine does and that often explains things.

  35. Matt Craighead says:

    If we really want to criticize the API design in any detail, I would also point out that getting the entire environment block is very rarely what you want.  Nor is the format of the environment block very application-friendly to parse, and there are plenty of opportunities for bugs there, too (can you say case sensitivity problems?).

    I would rather keep the format and order of the environment block hidden (so you’re free to change it — what if you wanted to change it to a hash table or binary tree to increase performance?), and only provide getenv/setenv APIs.  There is then the question of providing an environment block at process creation, but there, I expect you generally either want to (A) inherit everything or (B) build a new environment from scratch, selectively inheriting using getenv() if necessary.

  36. M1EK says:

    Ding ding ding for Matt Craighead’s answer.

    Also, you don’t need a time machine to identify that more than one party was to blame here. That doesn’t change the solution now, but it does help us possibly avoid future problems of this type.

    A programming API that requires that every programmer have read every single line of the manual to avoid dying in flames is not a good API. There was a time I felt differently, of course. I got better.

    [You didn’t have to read every single line of the manual. The information was right there in the documentation for GetEnvironmentStrings: “When the block returned by GetEnvironmentStrings is no longer needed, it should be freed by calling the FreeEnvironmentStrings function.” And besides, when you call a function that returns some sort of resource, don’t you naturally want to know “What do I do when I’m done with it?” -Raymond]
  37. James says:

    You don’t need to read every line of the manual in this case.

    You have a function that returns a pointer (and a non-const one, no less).  People with common sense should ask, "who owns the pointee, and if it’s the caller’s responsibility to free it, how does the caller do so?".  You don’t even need to look at the manual first to know to ask these questions; they’re implied by the function signature.

    A quick scan through the documentation should answer those questions.

    But sadly, I admit that common sense is not nearly as common as it ought to be.

  38. 640k says:

    Windows APIs has a lot of magic, can’t be sure about anything.

    C functions which returns buffers are always tricky, easy to get it wrong both when implementing the api code and the application code.

    "const" keyword doesn’t compile to anything, and isn’t enforced when executing.

Comments are closed.

Skip to main content