Re: secur32: check for variable before freeing it

[Henri Verbeet]

2009/10/13 Austin English :
>          cred = schan_free_handle(i, SCHAN_HANDLE_CRED);
> -
>        pgnutls_certificate_free_credentials(cred->credentials);
> -
>        if(cred) pgnutls_certificate_free_credentials(cred->credentials);

I don't think "cred" is ever supposed to be NULL. Could you add some
traces to see where that handle comes from?

secur32: check for variable before freeing it

[Vitaliy Margolen]

Henri Verbeet wrote:
> 2009/10/13 Austin English :
> >          cred = schan_free_handle(i, SCHAN_HANDLE_CRED);
> > -
> >        pgnutls_certificate_free_credentials(cred->credentials);
> > -
> >        if(cred) pgnutls_certificate_free_credentials(cred->credentials);
>
> I don't think "cred" is ever supposed to be NULL. Could you add some
> traces to see where that handle comes from?

This is the place Steam silently crashes on exit (found it last night with
valgrind):

==21864== Thread 1:
==21864== Invalid read of size 4
==21864==    at 0x153668B5: SECUR32_deinitSchannelSP (schannel.c:1438)
==21864==    by 0x1536BBFA: DllMain (secur32.c:675)
==21864==    by 0x1536FF6D: __wine_spec_dll_entry (dll_entry.c:40)
==21864==    by 0x7BC47674: ??? (in
/usr/local/src/wine.git-build/dlls/ntdll/ntdll.dll.so)
==21864==    by 0x7BC49672: MODULE_InitDLL (loader.c:985)
==21864==    by 0x7BC4A1CA: process_detach (loader.c:1156)
==21864==    by 0x7BC4ACDC: LdrUnloadDll (loader.c:2387)
==21864==    by 0x7B865A74: FreeLibrary (module.c:1023)
==21864==    by 0x30011254: ??? (in
/home/vitaliy/.wine-steam/drive_c/Program Files/Steam/Steam.dll)
==21864==    by 0x30004EC8: ??? (in
/home/vitaliy/.wine-steam/drive_c/Program Files/Steam/Steam.dll)
==21864==    by 0x301121F0: ??? (in
/home/vitaliy/.wine-steam/drive_c/Program Files/Steam/Steam.dll)
==21864==    by 0x4119F4: ??? (in /home/vitaliy/.wine-steam/drive_c/Program
Files/Steam/steam.exe)
==21864==  Address 0x4 is not stack'd, malloc'd or (recently) free'd

Vitaliy.

secur32: check for variable before freeing it

[Austin English]

On Tue, Oct 13, 2009 at 8:30 AM, Vitaliy Margolen
 wrote:
> Henri Verbeet wrote:
> > 2009/10/13 Austin English :
> > > cred = schan_free_handle(i, SCHAN_HANDLE_CRED);
> > > - pgnutls_certificate_free_credentials(cred->credentials);
> > > - if(cred) pgnutls_certificate_free_credentials(cred->credentials);
> >
> > I don't think "cred" is ever supposed to be NULL. Could you add some
> > traces to see where that handle comes from?

I'll take a look tonight.
> This is the place Steam silently crashes on exit (found it last night with
> valgrind):
>
> ==21864== Thread 1:
> ==21864== Invalid read of size 4
> ==21864==    at 0x153668B5: SECUR32_deinitSchannelSP (schannel.c:1438)
> ==21864==    by 0x1536BBFA: DllMain (secur32.c:675)
> ==21864==    by 0x1536FF6D: __wine_spec_dll_entry (dll_entry.c:40)
> ==21864==    by 0x7BC47674: ??? (in
> /usr/local/src/wine.git-build/dlls/ntdll/ntdll.dll.so)
> ==21864==    by 0x7BC49672: MODULE_InitDLL (loader.c:985)
> ==21864==    by 0x7BC4A1CA: process_detach (loader.c:1156)
> ==21864==    by 0x7BC4ACDC: LdrUnloadDll (loader.c:2387)
> ==21864==    by 0x7B865A74: FreeLibrary (module.c:1023)
> ==21864==    by 0x30011254: ??? (in
> /home/vitaliy/.wine-steam/drive_c/Program Files/Steam/Steam.dll)
> ==21864==    by 0x30004EC8: ??? (in
> /home/vitaliy/.wine-steam/drive_c/Program Files/Steam/Steam.dll)
> ==21864==    by 0x301121F0: ??? (in
> /home/vitaliy/.wine-steam/drive_c/Program Files/Steam/Steam.dll)
> ==21864==    by 0x4119F4: ??? (in /home/vitaliy/.wine-steam/drive_c/Program
> Files/Steam/steam.exe)
> ==21864==  Address 0x4 is not stack'd, malloc'd or (recently) free'd

What OS? 32/64-bit?

secur32: check for variable before freeing it

[Vitaliy Margolen]

Austin English wrote:
> What OS? 32/64-bit?

64-bit, Wine of course is 32-bit.

Vitaliy.

secur32: check for variable before freeing it

[Austin English]

On Tue, Oct 13, 2009 at 11:12 AM, Austin English
 wrote:
> On Tue, Oct 13, 2009 at 8:30 AM, Vitaliy Margolen
>  wrote:
> > Henri Verbeet wrote:
> > > 2009/10/13 Austin English :
> > > > cred = schan_free_handle(i, SCHAN_HANDLE_CRED);
> > > > - pgnutls_certificate_free_credentials(cred->credentials);
> > > > - if(cred) pgnutls_certificate_free_credentials(cred->credentials);
> > >
> > > I don't think "cred" is ever supposed to be NULL. Could you add some
> > > traces to see where that handle comes from?
>
> I'll take a look tonight.

static void *schan_free_handle(ULONG_PTR handle_idx, enum
schan_handle_type type)
{
    struct schan_handle *handle;
    void *object;

if (handle_idx == SCHAN_INVALID_HANDLE) return NULL;
    if (handle_idx >= schan_handle_count) return NULL;

Seems NULL comes from 'handle_idx >= schan_handle_count'.

secur32: check for variable before freeing it

[Juan Lang]

> Seems NULL comes from 'handle_idx >= schan_handle_count'.

Right, and that came about due to a double free that Steam seems to like to do.
--Juan

secur32: check for variable before freeing it

[Vitaliy Margolen]

Juan Lang wrote:
> > Seems NULL comes from 'handle_idx >= schan_handle_count'.
>
> Right, and that came about due to a double free that Steam seems to like to do.

It could be a false alarm with Steam, after all it's a "silent" crash <g> No
memory dump file is created either. Was pointing out a possible test app.

Vitaliy.

secur32: check for variable before freeing it

[Henri Verbeet]

2009/10/14 Austin English :
> static void *schan_free_handle(ULONG_PTR handle_idx, enum
> schan_handle_type type)
> {
>    struct schan_handle *handle;
>    void *object;
>
> if (handle_idx == SCHAN_INVALID_HANDLE) return NULL;
>    if (handle_idx >= schan_handle_count) return NULL;
>
> Seems NULL comes from 'handle_idx >= schan_handle_count'.

How does the attached patch work?

secur32: check for variable before freeing it

[Austin English]

On Wed, Oct 14, 2009 at 4:07 AM, Henri Verbeet  wrote:
> 2009/10/14 Austin English :
> > static void *schan_free_handle(ULONG_PTR handle_idx, enum
> > schan_handle_type type)
> > {
> >    struct schan_handle *handle;
> >    void *object;
> >
> > if (handle_idx == SCHAN_INVALID_HANDLE) return NULL;
> >    if (handle_idx >= schan_handle_count) return NULL;
> >
> > Seems NULL comes from 'handle_idx >= schan_handle_count'.
>
> How does the attached patch work?

Works here, thanks.