Valik Posted January 30, 2012 Share Posted January 30, 2012 (edited) 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 popupBOOL 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 Link to comment Share on other sites More sharing options...
Mat Posted January 30, 2012 Share Posted January 30, 2012 (edited) 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 AutoIt Project Listing Link to comment Share on other sites More sharing options...
Valik Posted January 30, 2012 Author Share Posted January 30, 2012 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. Link to comment Share on other sites More sharing options...
jaberwacky Posted January 30, 2012 Share Posted January 30, 2012 (edited) 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 Helpful Posts and Websites: AutoIt3 Variables and Function Parameters MHz | AutoIt Wiki | Using the GUIToolTip UDF BrewManNH | Can't find what you're looking for on the Forum? Link to comment Share on other sites More sharing options...
Valik Posted January 30, 2012 Author Share Posted January 30, 2012 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 Link to comment Share on other sites More sharing options...
wraithdu Posted January 30, 2012 Share Posted January 30, 2012 Well first off, assuming there's not another header somewhere, all your #defines are commented out... Link to comment Share on other sites More sharing options...
wraithdu Posted January 30, 2012 Share Posted January 30, 2012 And the handle to hToken is never closed. Link to comment Share on other sites More sharing options...
Valik Posted January 30, 2012 Author Share Posted January 30, 2012 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. Link to comment Share on other sites More sharing options...
wraithdu Posted January 30, 2012 Share Posted January 30, 2012 (edited) 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 Link to comment Share on other sites More sharing options...
Valik Posted January 31, 2012 Author Share Posted January 31, 2012 That was not one of the issues I was referring to. Link to comment Share on other sites More sharing options...
jaberwacky Posted January 31, 2012 Share Posted January 31, 2012 (edited) // 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 Helpful Posts and Websites: AutoIt3 Variables and Function Parameters MHz | AutoIt Wiki | Using the GUIToolTip UDF BrewManNH | Can't find what you're looking for on the Forum? Link to comment Share on other sites More sharing options...
jaberwacky Posted January 31, 2012 Share Posted January 31, 2012 (edited) 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 Helpful Posts and Websites: AutoIt3 Variables and Function Parameters MHz | AutoIt Wiki | Using the GUIToolTip UDF BrewManNH | Can't find what you're looking for on the Forum? Link to comment Share on other sites More sharing options...
Valik Posted January 31, 2012 Author Share Posted January 31, 2012 You really should cross-reference what the flags mean and think about them. There's nothing wrong with how the flags are handled. Link to comment Share on other sites More sharing options...
jaberwacky Posted January 31, 2012 Share Posted January 31, 2012 (edited) 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 Helpful Posts and Websites: AutoIt3 Variables and Function Parameters MHz | AutoIt Wiki | Using the GUIToolTip UDF BrewManNH | Can't find what you're looking for on the Forum? Link to comment Share on other sites More sharing options...
Valik Posted January 31, 2012 Author Share Posted January 31, 2012 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. Link to comment Share on other sites More sharing options...
jaberwacky Posted January 31, 2012 Share Posted January 31, 2012 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? Helpful Posts and Websites: AutoIt3 Variables and Function Parameters MHz | AutoIt Wiki | Using the GUIToolTip UDF BrewManNH | Can't find what you're looking for on the Forum? Link to comment Share on other sites More sharing options...
wraithdu Posted January 31, 2012 Share Posted January 31, 2012 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. Link to comment Share on other sites More sharing options...
Valik Posted January 31, 2012 Author Share Posted January 31, 2012 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. Link to comment Share on other sites More sharing options...
jaberwacky Posted January 31, 2012 Share Posted January 31, 2012 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; Helpful Posts and Websites: AutoIt3 Variables and Function Parameters MHz | AutoIt Wiki | Using the GUIToolTip UDF BrewManNH | Can't find what you're looking for on the Forum? Link to comment Share on other sites More sharing options...
Valik Posted January 31, 2012 Author Share Posted January 31, 2012 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. Link to comment Share on other sites More sharing options...
Recommended Posts
Create an account or sign in to comment
You need to be a member in order to leave a comment
Create an account
Sign up for a new account in our community. It's easy!
Register a new accountSign in
Already have an account? Sign in here.
Sign In Now