Fixes to win32 services #129
Conversation
There were quite a few issues with the win32 service code that this corrects. The first is that some of the comments in the code needed to be updated. Looks like code was copied and reused but the comments were not updated to reflect what the reused code was doing. There was the potential in InstallService() where not all the service handles would be closed if errors were hit at certain spots. Before installing a new service the old service was not uninstalled. This is desireable in the case where the new service has different options or points to a different path location for example. In some cases it might be bad where some type of user change was made but that is difficult to account for. I leaned toward cleaning up the old so that the new service can be installed fresh. This also causes an error when the service goes to install because the service already exists. This would actaully happen each time the OSSEC installer was ran but due to some incorrect logging statements (which I'll explain below) a blank line would appear. When doing an uninstall of a service the service wasn't stopped prior to the uninstallation. This would leave the service running until the service was stopped or the computer rebooted at which point the service would dissappear. It is better to stop the service before unintsalling. I'd imagine that is what the user would expect to happen during such an operation. The logging in this code was not done correctly. Namely, the call to merror() in the InstallService() function after the "install_error" label was completely wrong and would result in a nearly blank line in the logs. There were also reports of times where a user would install the agent on a win32 machine and everything would work except the service would never register. Fixing all of the logging to use verbose() should hopeflly lead to better troubleshooting of errors like that in the future.
|
Here is an example of the blank log line when doing an upgrade: Here is what things look like now when installing/uninstalling:
|
|
Fixing this will also help make error checking in |
|
Would it not be easier to let NSIS handle the service installation/upgrade? I had been meaning to look into that. It's a pretty simple operation that the installer should be capable of. |
|
I thought about that as well. There are quite a few plugins that make this pretty simple. You do lose the functionality of doing the |
|
Looks like |
|
I can't think of a good use case for someone needing to have a manual method to do that. There is always sc, regedit or psservice for power users, or if the installation becomes corrupted (which I have never seen), just reinstall then uninstall. |
|
Sounds good to me. I'm not sure when I personally will get around to putting in the necessary plugins into place so service setup can happen in NSIS. That said I still think this should get merged as it fixes other issues other than just the install/uninstall service code and hopefully fixes things in the interim while we move this logic into NSIS. Also, I know that people have reported on the mailing list that they had issues where the installer failed to register the service for whatever reason. The command line option may be useful in those cases but I agree that there are plenty other tools and they can likely just run the installer again. https://groups.google.com/forum/#!topic/ossec-list/9LiZpZ-Khd4 There really needs to be better error checking in the |
There were quite a few issues with the win32 service code that this
corrects. The first is that some of the comments in the code needed
to be updated. Looks like code was copied and reused but the comments
were not updated to reflect what the reused code was doing.
There was the potential in InstallService() where not all the service
handles would be closed if errors were hit at certain spots.
Before installing a new service the old service was not uninstalled.
This is desireable in the case where the new service has different
options or points to a different path location for example. In some
cases it might be bad where some type of user change was made but that
is difficult to account for. I leaned toward cleaning up the old so that
the new service can be installed fresh.
This also causes an error when the service goes to install because the service
already exists. This would actaully happen each time the OSSEC installer was ran
but due to some incorrect logging statements (which I'll explain below) a blank
line would appear.
When doing an uninstall of a service the service wasn't stopped prior to
the uninstallation. This would leave the service running until the service
was stopped or the computer rebooted at which point the service would dissappear.
It is better to stop the service before unintsalling. I'd imagine that is what
the user would expect to happen during such an operation.
The logging in this code was not done correctly. Namely, the call to merror()
in the InstallService() function after the "install_error" label was completely
wrong and would result in a nearly blank line in the logs. There were also reports
of times where a user would install the agent on a win32 machine and everything
would work except the service would never register. Fixing all of the logging to
use verbose() should hopeflly lead to better troubleshooting of errors like that
in the future.