Re: [PATCH] svchost: Implementation of svchost (revised submission)

[Roy Shea]

On Tue, Nov 27, 2007 at 03:37:50PM -0600, James Hawkins wrote:
> On Nov 27, 2007 3:21 PM, Roy Shea  wrote:
> > This is a revised and standalone version of svchost patch.  Changes in
> > this revision include:
> > - Unicode based with a simple UNICODE to ASCII function used to
> > convert the lpProcName passed into GetProcAddress
>
> I think you're misunderstanding Wine's policy about not using ansi
> functions or constant strings.  That only applies to external APIs
> because we don't want to convert from unicode to ansi (lossy).
> There's nothing wrong with just using:
>
> GetProcAddress(library, "ServiceMain");
>
> There aren't many instances where W->A is valid.

The name of the procedure passed into GetProcAddress is not always the
constant string "ServiceMain".  In these cases I'm getting the
procedure name from RegQueryValueEx.  RegQueryValueEx requires a
little bit of error checking to ensure proper NULL termination of a
REG_SZ.  I'd already wrapped this into the helper function GetRegValue
and thought a simple conversion from UnicodeToAscii would be cleaner
than writing a second ANSI version of GetRegValue.

I guess that inlining a native ANSI query doesn't look too bad:

----
/* Look for alternate to default ServiceMain entry point */
ret = RegQueryValueExA(service_hkey, service_main, NULL, NULL, NULL, &size);
if (ret != ERROR_SUCCESS)
{
    dll_service_main = NULL;
}
else
{
    dll_service_main = HeapAlloc(GetProcessHeap(), 0, size);
    ret = RegQueryValueExA(service_hkey, service_main, NULL, NULL,
            (LPBYTE)dll_service_main, &size);
    if (ret != ERROR_SUCCESS)
    {
        HeapFree(GetProcessHeap(), 0, dll_service_main);
        RegCloseKey(service_hkey);
        goto cleanup;
    }
    if (dll_service_main[size-1] != '\0')
    {
        WINE_TRACE("NULL terminating registry entry: %s\n", service_main);
        tmp_value = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 
                dll_service_main, size+1);
        if (!tmp_value)
        {
            HeapFree(GetProcessHeap(), 0, dll_service_main);
            RegCloseKey(service_hkey);
            goto cleanup;
        }
        dll_service_main = tmp_value;
    }
}
RegCloseKey(service_hkey);

/* Load the DLL and obtain a pointer to ServiceMain entry point */
library = LoadLibraryW(dll_name_long);
if (!library)
{
    WINE_ERR("failed to load library %s, err=%u\n",
            wine_dbgstr_w(dll_name_long), GetLastError());
    goto cleanup;
}
if (dll_service_main)
{
    service_main_func =
        (LPSERVICE_MAIN_FUNCTIONW) GetProcAddress(library, dll_service_main);
}
else
{
    service_main_func =
        (LPSERVICE_MAIN_FUNCTIONW) GetProcAddress(library, service_main);
}
if (!service_main_func)
{
    WINE_ERR("cannot locate ServiceMain procedure in DLL for %s\n",
            wine_dbgstr_w(service_name));
    FreeLibrary(library);
    goto cleanup;
}
----

I'll switch svchost over to a direct ANSI query when looking up the
DLL's service main function.
> > - Using HEAP_ZERO_MEMORY flag in calls to HeapAlloc and HeapReAlloc
> > that are allocating space for a string and the NULL terminator to
> > set the NULL terminator to zero.
>
> Why HEAP_ZERO_MEMORY when you can just set the last byte to NULL?
>
> str[LEN - 1] = '\0';

I was saving a line of code at the cost of performance.  My bad.  Thanks for
catching this.  And thank you for your continued help with this patch.

Peace,
-Roy