Can you find the bug?

[Valik]

I just found not one but 2 bugs that have been in AutoIt for years. Admittedly they are minor but they are still pretty basic bugs. Here's the function in question (as it appears in the 3.1.0 3.3.8.1 source). Can you spot both bugs?

expandcollapse popup

BOOL Util_Shutdown(int nFlag, int nReason)
{

/*
flags can be a combination of:
#define EWX_LOGOFF         0
#define EWX_SHUTDOWN         0x00000001
#define EWX_REBOOT         0x00000002
#define EWX_FORCE           0x00000004
#define EWX_POWEROFF         0x00000008        // need EWX_SHUTDOWN: now obsolete when using InitiateSystemShutdownEx()
#define EWX_FORCEIFHUNG   0x00000010 */

    HANDLE                hToken;
    TOKEN_PRIVILEGES    tkp;

    // If we are running NT, make sure we have rights to shutdown
    // Get a token for this process.
    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &hToken))
        return FALSE;                        // Don't have the rights

    // Get the LUID for the shutdown privilege.
    LookupPrivilegeValue(NULL, SE_SHUTDOWN_NAME, &tkp.Privileges[0].Luid);

    tkp.PrivilegeCount = 1;  /* one privilege to set */
    tkp.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;

    // Get the shutdown privilege for this process.
    AdjustTokenPrivileges(hToken, FALSE, &tkp, 0, (PTOKEN_PRIVILEGES)NULL, 0);

    // Cannot test the return value of AdjustTokenPrivileges.
    if (GetLastError() != ERROR_SUCCESS)
        return FALSE;                        // Don't have the rights

    // for suspend and hibernate code
    if ( (nFlag == 32) || (nFlag == 64) )
        return SetSystemPowerState(  (nFlag == 32),  FALSE);

    // Handle logging off the user.  This does not shutdown the machine.
    if ((nFlag & (EWX_SHUTDOWN|EWX_REBOOT|EWX_POWEROFF)) == 0)
        return ExitWindowsEx(nFlag, 0);

    BOOL bForceAppsClosed = FALSE;
    if (nFlag & (EWX_FORCE|EWX_FORCEIFHUNG))
        bForceAppsClosed = TRUE;

    BOOL bRebootAfterShutdown = FALSE;
    if (nFlag & EWX_REBOOT)
        bRebootAfterShutdown = TRUE;

    // Shutdown or restart the machine.
    return InitiateSystemShutdownEx(NULL, NULL, 0, bForceAppsClosed, bRebootAfterShutdown, nReason);

} // Util_Shutdown()

Edit: Updated to the 3.3.8.1 version of the code since it has changed slightly over the years.

Edited January 30, 2012 by Valik

[Mat]

I know you shouldn't use zero as the second parameter for ExitWindowsEx, but I'm guessing that doesn't really matter, as I don't know what else should go there.

(nFlag == 32) looks like it should be (nFlag & 32), I imagine the problem is trying to force suspend or something random like that.

Edit: Also, just saw this on msdn: If you are not the interactive user, use the InitiateSystemShutdown or InitiateSystemShutdownEx function.

Edited January 30, 2012 by Mat

[Valik]

I know you shouldn't use zero as the second parameter for ExitWindowsEx, but I'm guessing that doesn't really matter, as I don't know what else should go there.

(nFlag == 32) looks like it should be (nFlag & 32), I imagine the problem is trying to force suspend or something random like that.

Edit: Also, just saw this on msdn: If you are not the interactive user, use the InitiateSystemShutdown or InitiateSystemShutdownEx function.

Perhaps I should have shown the 3.3.8.1 version of the function as it is a bit different... Those are not the droids bugs you are looking for. Actually I'm updating the post with the version from 3.3.8.1 so red herrings like you found won't interfere with the real bugs.

[jaberwacky]

If I had to go on a limb and expose my c++ weakness I would say that the two bugs are using #define instead of const. If you want to pass a reference to tkp then doesn't tkp have to be declared a pointer? O rmaybe &hToken is passed in some functions but not in others? nReason is an int but InitiateSystemShutdownEx calls for a dword? SetSystemPowerState returns a dword and takes three arguments?

Wow, well everything seems valid until I post it. I give.

Edited January 30, 2012 by LaCastiglione

[Valik]

I'm begging to be disappointed. I saw the bigger of the two bugs instantly and the second one after a few moments of thinking about the code. Nobody can see them? o.o

[wraithdu]

Well first off, assuming there's not another header somewhere, all your #defines are commented out...

[wraithdu]

And the handle to hToken is never closed.

[Valik]

Well first off, assuming there's not another header somewhere, all your #defines are commented out...

That's because it's a comment. o.o No really, that's the point, to describe what values to expect, nothing more.

And the handle to hToken is never closed.

Yay, that's one and the most obvious bug. At least somebody understands resource leaks.

[wraithdu]

Another guess: ExitWindowsEx and InitiateSystemShutdownEx with a dwReason of 0 is not recommended. It would be suggested to use SHTDN_REASON_MAJOR_OTHER | SHTDN_REASON_MINOR_OTHER | SHTDN_REASON_FLAG_PLANNED. Although I can't see what the default value for nReason might be, so...

Edited January 30, 2012 by wraithdu

[Valik]

That was not one of the issues I was referring to.

[jaberwacky]

// Handle logging off the user.  This does not shutdown the machine.
    if ((nFlag & (EWX_SHUTDOWN|EWX_REBOOT|EWX_POWEROFF)) == 0)
        return ExitWindowsEx(nFlag, 0);

MSDN says: "Logs off the interactive user, shuts down the system, or shuts down and restarts the system." Is there a difference between system and machine that I don't see?

Edit: Scratch all of that, nvm.

Edited January 31, 2012 by LaCastiglione

[jaberwacky]

OK, my last try before I jump off the proverbial cliff. If nFlag is a bitmask then could you just use == on them like that or wouldn't use have to use bitwise operations??

Edited January 31, 2012 by LaCastiglione

[Valik]

You really should cross-reference what the flags mean and think about them. There's nothing wrong with how the flags are handled.

[jaberwacky]

There cannot be a flag of 32 but of 31? No, you said it isn't in how they are handled. So, we can assumne that, "You really should cross-reference what the flags mean and think about them," is a hint? Edited January 31, 2012 by LaCastiglione

[Valik]

No, it means you keep barking up the wrong tree trying to find a problem where there isn't one. Some of those flag combinations are invalid. For example, you never specify any other flag when you want to Standby (32) or HIbernate (64) the system therefore you always explicitly test those values using equality rather than bitwise. Et cetera.

[jaberwacky]

Ah, a comment says, "If we are running NT, make sure we have rights to shutdown" and so I tend to think that because AutoIt no longer supports anything below NT that there must be some invalid logic associated with that?

[wraithdu]

I keep looking for something obvious... but lacking that... I suppose, considering the function might be performing a standby call and not actually ending the process with a shutdown, that the function should first be querying the state of the SE_SHUTDOWN_NAME privilege, only enable it if necessary, and revert to the previous state after a call to standby or hibernate or if any of the shutdown calls fails for any reason.

[Valik]

I keep looking for something obvious... but lacking that... I suppose, considering the function might be performing a standby call and not actually ending the process with a shutdown, that the function should first be querying the state of the SE_SHUTDOWN_NAME privilege, only enable it if necessary, and revert to the previous state after a call to standby or hibernate or if any of the shutdown calls fails for any reason.

It's the "revert the previous state" part that's a bug. It leaves SE_SHUTDOWN_NAME enabled but under a default configuration that privilege should be disabled.

So to sum it up, the two bugs are:

• The hToken handle is never closed which in Standby and Hibernate lead to a memory leak.
• The SE_SHUTDOWN_NAME privilege is not reset back to it's default state. It's a trivial matter but it is still an error.

The solution, of course, is to encapsulate the behavior in a C++ class that provides a guaranteed destructor call which will take care of resetting the privilege(s) and closing the handle. Not to mention it hides all that nasty LookupPrivilege() and AdjustTokenPrivileges() non-sense that is pretty boilerplate. The destructor is extra nice in this case because not only does it avoid the assumption "Oh the process will close so these leaks don't matter" which is my guess as to why hToken leaks, it also means you don't have to write code to free the resources at the four separate exit points in the function.

Technically I guess there is a 3rd bug. That version of the function does not support a thread-level impersonation token since it goes straight for the process token. That is also fixed by a class that properly checks the thread token first.

Remember, C++ is your friend.

[jaberwacky]

OK, so not just any user will be able to run a program which will shut down the machine right? Is that what this code does? It finds out if the user has the right privileges?

// If we are running NT, make sure we have rights to shutdown

    // Get a token for this process.

    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &hToken))

        return FALSE;                        // Don't have the rights



    // Get the LUID for the shutdown privilege.

    LookupPrivilegeValue(NULL, SE_SHUTDOWN_NAME, &tkp.Privileges[0].Luid);



    tkp.PrivilegeCount = 1;  /* one privilege to set */

    tkp.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;



    // Get the shutdown privilege for this process.

    AdjustTokenPrivileges(hToken, FALSE, &tkp, 0, (PTOKEN_PRIVILEGES)NULL, 0);



    // Cannot test the return value of AdjustTokenPrivileges.

    if (GetLastError() != ERROR_SUCCESS)

        return FALSE;

[Valik]

No, it enables the privilege pretty much all users have unless the administrator explicitly removed a privilege from their account. Privileges can be in 3 states: Not granted, granted but disabled, granted but enabled. Permissions that are granted but disabled must be enabled with the code shown in the function. They will not be automatically enabled by whatever function(s) needs the privilege.