Windows could not start MYService on Local Computer

Joseph M. Newcomer

2011-06-22 16:09:16 UTC

See below....

Post by Rasheed
Hi ,
i have created a simple Service by reading the MSDN. i have installed
the service successfully, but when i try to start the service its

****
I do not think this is an exception being thrown. It strongly resembles a message from
the Service Control Manager.
*****

Post by Rasheed
Windows could not start the MYService on Local Computer.
Error 1053: The service did not respond to the start or control
request in a timely fashion.
i have pasted my code in below, kindly can anybody help me where i
did
wrong.
i am working on Windows 7 OS and VS 2010.
TCHAR szCommand[10];
TCHAR szSvcName[80];
#define SERVICENAME _T("MYService")
SC_HANDLE schSCManager;
SC_HANDLE schService;
VOID SvcInstall();
VOID DisplayUsage(void);
VOID StartSvc(void);
VOID uninstallSvc();
VOID Run();
VOID Run_Main(DWORD dwArgCount, LPTSTR args[]);

****
It helps readability a lot if you do things like put in
//*************************
// Forward Declarations
VOID SvcInstall();
...
VOID Run_Main(...);
//**************************

Good comments and vertical whitespace are essential!
*****

Post by Rasheed
SERVICE_TABLE_ENTRY s_DispatchTable[] =
{
{ (LPWSTR)SERVICENAME, (LPSERVICE_MAIN_FUNCTION)Run_Main },

****
There is no sane reason to write the (LPWSTR) or (LPSERVICE_MAIN_FUNCTION) casts here. In
fact, it is potentially erroneous to do so. If it won't compile without these casts, you
have deeper problems which may explain why it isn't starting. Remove them.
******

Post by Rasheed
{ NULL, NULL } };
void _tmain(int argc, TCHAR *argv[])
{
StringCchCopy(szCommand, 10, argv[1]);

*****
This makes no sense because there are no arguments to _tmain in a service, and in fact
argc is 1, so accessing argv[1] is nonsensical. And is probably why the service is
failing, because it is probably a NULL or other illegal pointer!

This weird hybrid code is bizarre beyond belief, and if you copied this from the MSDN,
then it is an example that must go into my "Unbelievably bad MSDN examples" code!

So first thing, before the StringCchCopy, is make sure there IS an argv[1].

Second, the StringCchCopy is COMPLETELY POINTLESS and serves no discernable useful
purpose, other that wasting time, cluttering up the code, and causing potential bufer
overruns (why is it that you assume that there needs to be a variable szCommand, why is it
foolishly declared as a global variable, and why do you assume that the "10" is the actual
size?

Get rid of the variable. Get rid of the StringCchCopy.
*****

Post by Rasheed
if (lstrcmpi( szCommand, TEXT("install")) == 0 )
SvcInstall();

*****
Note that this could have easily been written
if(lstrcmpi(argv[1], _T("install")) == 0)
and there is no need to introduce an additional variable to handle this case. And there
is NO POSSIBLE reason to have made it a global variable!
*****

Post by Rasheed
else if (lstrcmpi( szCommand, TEXT("start")) == 0 )
StartSvc();
else if (lstrcmpi( szCommand, TEXT("uninstall")) == 0 )
uninstallSvc();
else
{

****
Note that none of this code could possibly make sense; this is a feeble attempt to have "a
single executable" that is both the service and an interface to it. This approach is
foolish. You create a service, and it is a service, it is not a program that pretends
that sometimes it is a service and sometimes it isn't. You create a SECOND program to
interact with it. And there is no point to doing that, because the 'sc' program already
exists, so all of this code is pointless.

If this REALLY came from the MSDN, then it was written by someone whose claims to
competence as a programmer seriously need to be examined! And his/her manager should be
fired, also, for letting a piece of trash like this in the MSDN. If you invented this
yourself, then you probably didn't read the MSDN well enough. You might also check my MVP
Tips site, my essay on MSDN Errors and Omissions, because I've already found seriously
deep flaws in the MSDN service example (none of which I see here; the code I saw in the
MSDN is apalling, but correct; this code is just WRONG)
****

Post by Rasheed
Run();
_tprintf(TEXT("Unknown command (%s)\n\n"),
szCommand);

****
Why is it that an unknown command means "Run"? There's something wrong with the logic
here!
****

Post by Rasheed
DisplayUsage();

*****
If this is being invoked as a service, the StringCchCopy will crash the app with an access
fault (access to invalid memory address) since argv[1] will not be defined. And that
means that doing a _tprintf here is nonsensical, as is DisplayUsage. But fundamentally
the entire structure of this program is rubbish and it needs to have all this
command-line-parsing excised as gratuitous flab that serves no useful purpose.
****

Post by Rasheed
}
}
VOID Run()
{

*****
This code belongs in main/tmain.

Of course this has led to another failure, the use of a global variable. Why is
s_DispatchTable a global variable? There is no reason to make it so. It could be a local
here, or, more properly, in _tmain
*****

Post by Rasheed
BOOL started
= ::StartServiceCtrlDispatcher( s_DispatchTable );
if( !started)
{
}
}
void Run_Main( DWORD dwArgCount, LPTSTR args[] )

*****
Now, either this program IS a service, in which case there is practically nothing in
_tmain, or it is a program that manipulates a service, in which case Run_Main does not
belong in it!
*****

Post by Rasheed
{
;
}
VOID DisplayUsage()

****
Get rid of this. It serves no useful purpose. Note that you will NEVER, EVER use
printf/_tprintf in a system service.

Post by Rasheed
{
printf("Description:\n");
printf("\tCommand-line tool that controls a service.\n\n");
printf("Usage:\n");
printf("\tsvccontrol [command] [service_name]\n\n");
printf("\t[command]\n");
printf("\t start\n");
printf("\t dacl\n");
printf("\t stop\n");

****
Why is there an odd mix of _tprintf and printf calls?
****

Post by Rasheed
}
VOID SvcInstall()

****
Get rid of this entire function; it serves no useful purpose
****

Post by Rasheed
{
SC_HANDLE schSCManager;
SC_HANDLE schService;
TCHAR szPath[MAX_PATH];
if( !GetModuleFileName( NULL, szPath, MAX_PATH ) )
{
printf("Cannot install service (%d)\n", GetLastError());
return;
}
// Get a handle to the SCM database.
schSCManager = OpenSCManager(
NULL, // local computer
NULL, // ServicesActive database
SC_MANAGER_ALL_ACCESS); // full access rights
if (NULL == schSCManager)
{
printf("OpenSCManager failed (%d)\n", GetLastError());
return;
}
// Create the service.
schService = CreateService(
schSCManager, // SCM database
SERVICENAME, // name of service
SERVICENAME, // service name to display
SERVICE_ALL_ACCESS, // desired access
SERVICE_WIN32_OWN_PROCESS, // service type
SERVICE_DEMAND_START, // start type
SERVICE_ERROR_NORMAL, // error control type
szPath, // path to service's binary
NULL, // no load ordering group
NULL, // no tag identifier
NULL, // no dependencies
NULL, // LocalSystem account
NULL); // no password
if (schService == NULL)
{
printf("CreateService failed (%d)\n", GetLastError());
CloseServiceHandle(schSCManager);
return;
}
else printf("Service installed successfully\n");
CloseServiceHandle(schService);
CloseServiceHandle(schSCManager);
}
//
// Starts the service.
//
// None
//
// None
//
VOID StartSvc()

****
Get rid of this piece of trash; it does not belong in a system service
****

Post by Rasheed
{
SERVICE_STATUS_PROCESS ssStatus;
DWORD dwOldCheckPoint;
DWORD dwStartTickCount;
DWORD dwWaitTime;
DWORD dwBytesNeeded;
// Get a handle to the SCM database.
schSCManager = OpenSCManager(
NULL, // local computer
NULL, // ServicesActive database
SC_MANAGER_ALL_ACCESS); // full access rights
if (NULL == schSCManager)
{
printf("OpenSCManager failed (%d)\n", GetLastError());
return;
}
// Get a handle to the service
schService = OpenService(
schSCManager, // SCM database
szSvcName, // name of service
SERVICE_ALL_ACCESS); // full access
if (schService == NULL)
{
printf("OpenService failed (%d)\n", GetLastError());
CloseServiceHandle(schSCManager);
return;
}
// Attempt to start the service.
if (!StartService(
schService, // handle to service
0, // number of arguments
NULL) ) // no arguments
{
printf("StartService failed (%d)\n", GetLastError());
CloseServiceHandle(schService);
CloseServiceHandle(schSCManager);
return;
}
else printf("Service start pending...\n");
// Check the status until the service is no longer start pending.
if (!QueryServiceStatusEx(
schService, // handle to service
SC_STATUS_PROCESS_INFO, // info level
(LPBYTE) &ssStatus, // address of structure
sizeof(SERVICE_STATUS_PROCESS), // size of structure
&dwBytesNeeded ) ) // if buffer too small
{
return;
}
// Save the tick count and initial checkpoint.
dwStartTickCount = GetTickCount();
dwOldCheckPoint = ssStatus.dwCheckPoint;
while (ssStatus.dwCurrentState == SERVICE_START_PENDING)
{
// Do not wait longer than the wait hint. A good interval is
// one-tenth the wait hint, but no less than 1 second and no
// more than 10 seconds.
dwWaitTime = ssStatus.dwWaitHint / 10;
if( dwWaitTime < 1000 )
dwWaitTime = 1000;
else if ( dwWaitTime > 10000 )
dwWaitTime = 10000;
Sleep( dwWaitTime );
// Check the status again.
if (!QueryServiceStatusEx(
schService, // handle to service
SC_STATUS_PROCESS_INFO, // info level
(LPBYTE) &ssStatus, // address of structure
sizeof(SERVICE_STATUS_PROCESS), // size of structure
&dwBytesNeeded ) ) // if buffer too small
break;
if ( ssStatus.dwCheckPoint > dwOldCheckPoint )
{
// The service is making progress.
dwStartTickCount = GetTickCount();
dwOldCheckPoint = ssStatus.dwCheckPoint;
}
else
{
if(GetTickCount()-dwStartTickCount > ssStatus.dwWaitHint)
{
// No progress made within the wait hint.
break;
}
}
}
// Determine whether the service is running.
if (ssStatus.dwCurrentState == SERVICE_RUNNING)
{
printf("Service started successfully.\n");
}
else
{
printf("Service not started. \n");
printf(" Current State: %d\n", ssStatus.dwCurrentState);
printf(" Exit Code: %d\n", ssStatus.dwWin32ExitCode);
printf(" Check Point: %d\n", ssStatus.dwCheckPoint);
printf(" Wait Hint: %d\n", ssStatus.dwWaitHint);
}
CloseServiceHandle(schService);
CloseServiceHandle(schSCManager);
}
void uninstallSvc()

****
Get rid of thsi function, it is completely inappropriate
****

Post by Rasheed
{
if( schSCManager == 0 )

****
I am curious why you are comparing this value to 0. The documentation CLEARLY states the
return of OpenSCManager is NULL, not 0. Stop using 0 where NULL should be written!

Now, why, when you get here, do you expect that schSCManager could POSSIBLY be non-NULL?

First, there is no reason, other than exceptionally bad style, that this should be a
global variable at all. Second, the ONLY POSSIBLE path that leads here comes from _tmain,
where there are no assignments to schSCManager before uninstallSvc is called.

So the test is silly and pointeless, and the existence of the global variable represents
amazingly bad style. If there were any point to rewriting this code (which there isn't;
it should be discarded and never seen again, anywhere), schSCManager would be a local
variable in this function, and therefore the test would be meaningless until after it is
opened, e.g.,

void UninstallSvc()
{
SC_HANDLE schSCManager = ::OpenSCManager(...);
if(schSCManager == NULL)
{
... deal with error
return;
}
...
}

This looks like bad copy-and-paste from an article written by a clueless programmer.
****

Post by Rasheed
{
schSCManager
= ::OpenSCManager( NULL, // local
computer
NULL, // ServicesActive database
SC_MANAGER_ALL_ACCESS); // full access rights
if (NULL == schSCManager)
{
//_tprintf(_T("OpenSCManager failed,
error: %u
\n"), ::GetLastError() );
return;
}
}
if( schService == 0 )

****
This, of course, is complete nonsense; if the handle is NULL (erroneously coded here as ==
0) then the OpenService call will fail! If it had said != NULL, then the if-statement
would make sense. But since this entire function is rubbish and does not belong in this
module, and there is no need to ever have it written, anywhere (because of the sc
command), this is a minor point.
*****

Post by Rasheed
{
schService
= ::OpenService( schSCManager,SERVICENAME ,
SC_MANAGER_ALL_ACCESS );
if( schService == 0 )

****
Similarly, OpenService CLEARLY states the return value is NULL if there is a failure, so
writing == 0 is exceptionally poor style!
****

Post by Rasheed
{
//_tprintf( SERVICENAME _T(" not found.
\n") );
}
}
if( schService != 0 )

****
Why not just write 'else' here?
****

Post by Rasheed
{
BOOL ok = ::DeleteService( schService );
if( ok )
{
_tprintf( _T("Service deleted\n") );
}
else
{
DWORD dwError = ::GetLastError();
_tprintf( _T("DeleteService failed,
error: %u\n"), dwError );
}
::CloseServiceHandle( schService );
schService = 0;

****
Similarly, assigning 0 here is exceptionally bad style. Furthermore, schService does not
need to have a value such as NULL assigned to it, because it MUST be a local variable and
there is no conceivable reason to make it a global variable
****

Post by Rasheed
}
if( schSCManager != 0 )
::CloseServiceHandle( schSCManager );
}

****
Nearly all of this code should be thrown out. Most of the code I see here makes NO SENSE
in a service, and is completely and utterly inappropriate. The parts of the service that
matter, that would possibly lead to the error shown, are not displayed here, so there is
no way to tell. Throw out EVERYTHING that deals with parsing the command line, every
single line of that code is total trash. Build a service which is a SERVICE, not some
weird pointless hybrid that serves no discernable purpose and has no discernable value.
Then you concentrate on what it means to write a SERVICE and that includes calling
StartServiceControlDispatcher, which is typically the ONLY significant line in main/_tmain
(there's usually an if-statement that deals with what to do if THAT call fails, which is
usually to return from main/_tmain). Anything else is rubbish.

You may have learned an important lesson here: the MSDN examples are rarely written by
professionals who know what they are doing, and apparently never reviewed by anyone to see
if they are correct or sensible. They often embody particularly horrid programming style
misfeatures, such as the use of global variables, failure to use NULL when NULL is meant,
etc. They are often written in some kind of pseudo-code that doesn't even compile. The
few that appear to compile and "work" are written frequently unmaintainable; for example,
my critique of the "HTMLEditView" program example, which had to have been written by
someone who read Petzold's book and actually believed it taught correct programming style
(see my MVP Tips site for my critique of HTMLEditView), or the multithreaded socket
example that got socket programming, synchronization, and threading wrong (not bad,
three-for-three, except that some innocent programmer who worked for one of my clients
read it, copied it, and the result was the failure of a product into which millions had
been invested, because the code was not only not right, but could not be fixed except by
rewriting from the ground up)

Look at the sc command, which comes with every version of Windows since Windows 2000. And
what that means is you truly throw out all this code and never bother to incorporate it
into another program.

A service is a service. It does nothing else. A program which manipulates (installs,
starts, removes) a service is just that, a program with certain features to manipulate a
service. It is not the service itself. This example is one of the worst examples I have
seen of mixing functionality in the bizarre delusion that this "simplifies" things. Save
me from programmers who think techniques like this are cute!

Besides, it has such a fundamental bug in it that I'm amazed it could ever have been used
by anyone!
joe
*****

Post by Rasheed
Thanks in advance.
Rs.

Joseph M. Newcomer [MVP]
email: ***@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm