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

[James Hawkins]

On Nov 27, 2007 3:21 PM, Roy Shea <roy(a)cs.hmc.edu> 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.
> - 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';

-- 
James Hawkins

[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 <roy(a)cs.hmc.edu> 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