How am I supposed to free the information returned by the GetSecurityInfo function?


The Get­Security­Info function returns a copy of the security descriptor for a kernel object, along with pointers to specific portions you request. More than once, a customer has been confused by the guidelines for how to manage the memory returned by the function.

Let's look at what the function says:

ppsidOwner [out, optional]

A pointer to a variable that receives a pointer to the owner SID in the security descriptor returned in ppSecurity­Descriptor. The returned pointer is valid only if you set the OWNER_SECURITY_INFORMATION flag. This parameter can be NULL if you do not need the owner SID.

Similar verbiage can be found for the other subcomponent parameters. The final parameter is described as

ppSecurity­Descriptor [out, optional]

A pointer to a variable that receives a pointer to the security descriptor of the object. When you have finished using the pointer, free the returned buffer by calling the Local­Free function.

Okay, so it's clear that you need to free the security descriptor with Local­Free. But how do you free the owner, group, DACL, and SACL?

Read the documentation again. I've underlined the important part.

ppsidOwner [out, optional]

A pointer to a variable that receives a pointer to the owner SID in the security descriptor returned in ppSecurity­Descriptor. The returned pointer is valid only if you set the OWNER_SECURITY_INFORMATION flag. This parameter can be NULL if you do not need the owner SID.

In case that wasn't clear, the point is reiterated in the remarks.

If the ppsidOwner, ppsidGroup, ppDacl, and ppSacl parameters are non-NULL, and the Security­Info parameter specifies that they be retrieved from the object, those parameters will point to the corresponding parameters in the security descriptor returned in ppSecurity­Descriptor.

In other words, you are getting a pointer into the security descriptor. No separate memory allocation is made. The memory for the owner SID is freed when you free the security descriptor. It's like the last parameter to Get­Full­Path­Name, which receives a pointer to the file part of the full path. There is no separate memory allocation for that pointer; it's just a pointer back into the main buffer.

You can think of the ppsidOwner parameter as a convenience parameter. The Get­Security­Info function offers to do the work of calling Get­Security­Descriptor­Owner for you. You can think of the function as operating like this:

DWORD WINAPI GetSecurityInfo(...)
{
    ... blah blah get the security info ...

    // Just out of courtesy:
    // Fetch the owner if the caller requested it
    if (ppsidOwner != NULL &&
        (SecurityInfo & OWNER_SECURITY_INFO)) {
        BOOL fDefaulted;
        GetSecurityDescriptorOwner(pSecurityDescriptor,
                                   ppsidOwner,
                                   &fDefaulted);
    }

    ...
}

That's why the documentation says that you need to pass a non-null ppSecurity­Descriptor if you request any of the pieces of the security descriptor: If you don't, then you won't be able to free the memory for it.

Bonus chatter: If the ppSecurity­Descriptor is so important, why is it marked "optional"?

It really should be a mandatory parameter, but older versions of Windows didn't enforce the rule, so the parameter is grandfathered in as optional, even though no self-respecting program should ever pass in NULL. If you pass NULL for the ppSecurity­Descriptor, the function happily allocates the security descriptor and then, "Oh wait, the caller didn't give me a way to receive the pointer to the security descriptor, so I guess I won't give it to him."

DWORD WINAPI GetSecurityInfo(...)
{
    ... blah blah get the security info ...

    if (ppSecurityDescriptor != NULL) {
        *ppSecurityDescriptor = pSecurityDescriptor;
    }
    ...
}

Result: Memory leak.

You might say that the last parameter was designed by somebody wearing kernel-colored glasses.

Comments (25)
  1. I figured it out just by reading the documentation, before reading the actual blog entry.  Do I get a prize?

    What I didn't figure out from reading documentation was how there would be a memory leak if ppSecurityDescriptor is NULL!  I assume you would have pointed it out in the documentation if it was documented?  In which case, the documentation seems sorely lacking in this area – generally, I don't expect a memory leak if I decide I don't need the output of an optional output parameter!  (In this case, it doesn't seem to make sense to leave ppSecurityDescriptor NULL since there would be no way to get useful information other than an error code in that case.  But it still violates this rule I assumed was true.)

    [It may violate the rule, but the alternative is requiring the function to have clairvoyance. -Raymond]
  2. WndSks says:

    I don't really get this, how was it ever useful to allow people to pass NULL as the final parameter? It could not be to allow ppsidOwner to be a separate memory block in the future because the way you free things have already been set in stone.

    So my question is, why did the original design not just validate the pointer as its first step and return with a invalid parameter error? Also, has a "else LocalFree(pSecurityDescriptor);" been added in later versions or does it still leak?

    [I'd be more inclined to answer if your handle wasn't insulting. -Raymond]
  3. Anonymous says:

    WndSks = Windows Sockets?

    Filler. This is not a spam. Neither very useful, I know.

  4. Anonymous says:

    @prunoki, more like it Sucks. That user actually has as a homepage something with that in the URL. I believe this user and xpclient are friends. Their tone is incredibly similar too.

    I don't know why they don't just move on to something else. From their point of view (read that again), it's like being in hell with a chance to go to heaven, but no, they'd rather stay in hell pointing out how red, hot and painful it is, even with their life-saving-lava-temperature-resistant old-times-comfortable yacht.

  5. Anonymous says:

    @WndSks: That would technically work if the Win32 program were single threaded and LocalFree never ends up calling VirtualFree and there was no allocation in the process.

    Constraint #1 doesn't hold as soon as you call OpenFileDialog

  6. Anonymous says:

    [You'd be surprised how many people rely on the memory leak. -Raymond]

    Exactly. How many people can be expected to get "use memory after freeing it" correct?

  7. Anonymous says:

    I think the comments are overlooking the fact that even when ppSecurityDescriptor is NULL, the function still fills in the other parameters to point into the memory it allocated.  It doesn't return the pointer you need to free the memory, but at least the results are still valid.

  8. Anonymous says:

    I understand maintaining backward compatibility.  I don't understand getting the initial implementation wrong.

    [The original implementation was written in the days when the prevailing design style was "if the app wants to do something stupid, let them do something stupid, because preventing people from doing stupid things might prevent them from doing clever things." -Raymond]
  9. [So you're saying that the function should return *ppsidOwner that points to memory that has already been freed? (You'd be surprised how many people rely on memory leaks.) -Raymond]

    No, because MSDN says: "This parameter is required if any one of the ppsidOwner, ppsidGroup, ppDacl, or ppSacl parameters is not NULL."

    I read that and assumed the function will fail with an invalid parameter error code if ppSecurityDescriptor is NULL and ppsidOwner is not NULL.  If this is not the case, as Jeffrey Bosboom says, then you're right, someone could end up using freed memory – not good – better to leak the memory for backwards compatibility.  But the documentation probably should clarify things, because then the statement I just quoted is apparently false – in addition to the previously-mentioned lack of a warning about a memory leak.

    @Maurits:  Now I think you've needlessly complicated the function: you're requiring the caller to have the smarts to LocalFree(psidOwner) when they didn't have the smarts to call it right in the first place, and would break compatibility with older versions.  Otherwise, it will still leak.

    [Oh, so you're breaking code that accidentally leaked memory. They used to succeed with a valid psidOwner (and leak some memory), and now they get ERROR_INVALID_PARAMETER. The comment "This parameter is required…" was added by me in 2010. Documentation prior to 2010 did not explicitly note the parameter as mandatory. (The Microsoft Research people need to hurry up with that time machine.) -Raymond]
  10. Anonymous says:

    @JamesJohnston That's my interpretation of the documentation as well. Well since the memory leak only happens to applications that misuse the function, it's probably fine to leave it how it is – no downsides for people who use it correctly and a small memory leak is better than reading freed memory for all the others.

  11. Anonymous says:

    You can think of the ppsidOwner parameter as a convenience parameter…

    Haha. I guess this happens from time to time in APIs. Most developers won't get confused, one would assume, but no good deed goes unpunished as has been tagged in this blog from time to time.

  12. [It may violate the rule, but the alternative is requiring the function to have clairvoyance. -Raymond]

    Rule violations should be documented, IMHO, not sure why they didn't do that… (forgot?)

    Not sure what clairvoyance is needed, I don't see what would have been wrong to change the function like this?

       if (ppSecurityDescriptor != NULL) {

           *ppSecurityDescriptor = pSecurityDescriptor;

       } else {

           LocalFree(pSecurityDescriptor); /* caller left NULL in parameter; avoid memory leak */

       }

    Shouldn't really break compatibility with anything either, unless someone actually depended on the memory leak somehow. (!!!)

    [So you're saying that the function should return *ppsidOwner that points to memory that has already been freed? (You'd be surprised how many people rely on memory leaks.) -Raymond]
  13. Anonymous says:

    [The original implementation was written in the days when the prevailing design style was "if the app wants to do something stupid, let them do something stupid, because preventing people from doing stupid things might prevent them from doing clever things." -Raymond]

    Does this mean it's safe for me to assume I can call it with NULL, take one of the other pointers, hop backwards through memory to find the start of the security descriptor (never mind how), and call LocalFree on it? Not that I'd do anything that insane. Just interested in the correct way to read the documentation in case another odd matter comes up of a similar form.

    [No, that's not a good idea. As I noted in the article, this is a kernel-colored glasses issue. The function is careful not to crash itself, but it doesn't really care if what you requested harms you as a side-effect. If you want to hurt yourself, that's your problem. Think of it as the libertarian version of API documentation. -Raymond]
  14. There's lots of ways it *could* have worked.  For example:

    if (ppSecurityDescriptor == NULL) {

    ___ if (ppsidOwner != NULL) {

    ___ ___ *ppsidOwner = LocalAlloc(…);

    ___ ___ CopyMemory(ppsidOwner, …);

    ___ ___ LocalFree(pSecurityDescriptor);

    ___ }

    } else {

    ____ *ppSecurityDescriptor = pSecurityDescriptor;

    }

    [In general, you don't design your function so it behaves optimally when people are using it wrong. -Raymond]
  15. Anonymous says:

    @Joshua

    Might it work in a single configuration on which you test?  Yes.  Is it safe?  You already know the answer to that.

  16. Anonymous says:

    @Raymond:

    Could you arrange for the MSDN documentation to be updated so that the documentation does not say "optional" in the signature? You could note in the remarks of the function that some old applications send NULL to this function and that for app-compat the function will succeed with a memory leak (in order to prevent people shimming this function from null-dereffing), but since the vast majority of users of MSDN are trying to use rather than shim the API, and since memory leaking is a bug (i.e. sending NULL in this parameter is a bug), it seems reasonable for the signature to require this parameter and mark is as __out, not __out_opt.

    Additionally, if you parameter is changed to not be optional, then stuff like OACR will be able to alert users of static analysis tools that they are "doing it wrong" so that these leaks can be fixed.

  17. Anonymous says:

    @Joshua: Just because it works doesn't mean it's supported. And unless your app suddenly becomes a fifty-million-customers strong, you probably will find that Microsoft can't be bothered to write the shim for your app to keep it working when your app inevitably breaks on a future version of Windows.

    Note that in a future version of Windows where that offset becomes wrong or the innards of the method change, you'll suddenly be LocalFree'ing a pointer that isn't valid – which is an exploitable software vulnerability.

    And when your customers ask "how come a hacker was able to install a rootkit on my machine when I used your program" you'll have to reply "because I'm an idiot and relied on undocumented behaviour of a function".

  18. Anonymous says:

    Suppose a company is actively developing an application that happens to be incorrectly using GetSecurityInfo in this way. How are they supposed to notice that? (Apart from reading this blog, of course.)

  19. Anonymous says:

    @Neil, that's a good question.

    Usually, the MSDN library documentation states what error codes are returned and in what situations.

    The documentation (as Raymond quotes) always stated that ppSecurityDescriptor was needed if any of the other 4 were not NULL (and in fact, it wasn't marked as optional as far back as 2005, in the documentation and in the C declaration).

    In this case, it says ppSecurityDescriptor is required if the other 4 parameters are not NULL, so violating this alone should return an error code, and the guilty apps should have shims.

    As you said, whoever passes a NULL ppSecurityDescriptor might honestly be affected by a bug somewhere else in the application/library. Since no error is returned, there's no way to know.

    [As I noted earlier, the documentation that says that ppSecurityDescriptor is required if any of the other 4 parameters is non-NULL was added in 2010. So between 2005 and 2010, it was documented as optional. As I noted in the article, the parameter really is optional (from a kernel perspective) but the consequence is a memory leak so in practice it is mandatory. -Raymond]
  20. Anonymous says:

    @Joshua, the "workaround" would be too clever. I dare saying it would actually be too much cleverness for whoever uses an explicit NULL ppSecurityDescriptor.

    Call GetSecurityDescriptorOwner, GetSecurityDescriptorGroup, GetSecurityDescriptorDacl and/or GetSecurityDescriptorSacl once, given a properly filled SECURITY_DESCRIPTOR (i.e. with the respective field not set to NULL), to guess the offset.

    Now you know the offsets. But wait, in calls with a SECURITY_DESCRIPTOR with those fields set to NULL, the owner, group, dacl and sacl pointers will actually be set to NULL instead of set to point to the field that has the NULL value. You basically don't have a reliable way of not leaking memory.

    GetSecurityInfo itself has the ability to LocalFree the SECURITY_DESCRIPTOR (if ppSecurityDescriptor is NULL) if all the values stored in the other 4 pointers are set to NULL. Actually, it should, there's no need to leak in this case.

  21. Anonymous says:

    @John Doe: "I don't know why they don't just move on to something else."

    To be fair, the tag-line for WndSks's site is "Windows sucks, but everything else sucks even more". A statement I can almost get behind ;) But actually Raymond's post helped increase my appreciation for Windows quite a lot.

  22. Anonymous says:

    In general, you don't design your function so it behaves optimally when people are using it wrong. -Raymond

    A lovely bon-mot.

  23. Anonymous says:

    @Raymon, you're right about the added paragraph.

    However, in the Visual Studio 2005 installation in the machine I'm posting from, the installed MSDN library states that the ppSecurityDescriptor is [out], not [out, optional] (in AclAPI.h, it's __deref_out, not__deref_out_opt). No matter how the function actually worked/works, it was not a good idea to pass NULL for ppSecurityDescriptor back then. So, it seems the parameter was made optional to reflect its behavior, instead of the function made to work as documented.

    That's why I said the function should probably have been fixed and shims done to the apps that really require the old misdocumented/misimplemented behavior. But it's a bit too late now, it seems :)

    I guess that, in between 2005 and your addition, documenting ppSecurityDescriptor as optional was the main fault, no matter how the function was actually implemented.

  24. Anonymous says:

    As a matter of perspective, I agree on WndSks's post not being offensive. His blog's headline is "Windows sucks, but everything else sucks even more". That sounds more like some frustration from simple annoyances with his favorite OS.

    And one of his posts starts with "In the words of the great Raymond Chen:"

    Even if the rest is (educated) ranting, he's certainly right about that one.

  25. Anonymous says:

    "WndSks" isn't funny, clever or even the slightest bit ironic.  It's just rude.  Raymond is well correct to point this out. It's not unreasonable to expect a little common courtesy from those to whom you provide so much value.  For Free.

    Try this:

    1. Go to McDonald's

    2. Tell them the food they prepare sucks.

    3. Order some food.

    4. Enjoy the spit (or worse) that comes as a bonus condiment on your food.

Comments are closed.