Re: secur32: check for variable before freeing it

[Henri Verbeet]

2009/10/13 Austin English <austinenglish(a)gmail.com>:
> 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 <austinenglish(a)gmail.com>:
> > 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
<wine-devel(a)kievinfo.com> wrote:
> Henri Verbeet wrote:
> > 2009/10/13 Austin English <austinenglish(a)gmail.com>:
> > >              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?

-- 
-Austin

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
<austinenglish(a)gmail.com> wrote:
> On Tue, Oct 13, 2009 at 8:30 AM, Vitaliy Margolen
> <wine-devel(a)kievinfo.com> wrote:
> > Henri Verbeet wrote:
> > > 2009/10/13 Austin English <austinenglish(a)gmail.com>:
> > > >              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'.

-- 
-Austin

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 <austinenglish(a)gmail.com>:
> 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 <hverbeet(a)gmail.com> wrote:
> 2009/10/14 Austin English <austinenglish(a)gmail.com>:
> > 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.

-- 
-Austin