[Bug 21568] New: advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

Summary: advapi32/crypt tests crash with +heap
           Product: Wine
           Version: 1.1.37
          Platform: x86-64
               URL: http://test.winehq.org/data/7aaaf738ecd06c12bfd69200ee
                    74abd5cc9aef8b/wine_ae-ub910-heap/advapi32:crypt.html
        OS/Version: Linux
            Status: NEW
          Keywords: download, source, testcase
          Severity: minor
          Priority: P2
         Component: advapi32
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

Created an attachment (id=26007)
 --> (http://bugs.winehq.org/attachment.cgi?id=26007)
terminal output

=>0 0x7ec1ac0d CryptSetKeyParam+0x98(hKey=1334760, dwParam=4, pbData="",
dwFlags=4) [/home/austin/wine-git/dlls/advapi32/crypt.c:1961] in advapi32
(0x0063fcf8)
...
0x7ec1ac0d CryptSetKeyParam+0x98
[/home/austin/wine-git/dlls/advapi32/crypt.c:1961] in advapi32: movl
0x0(%eax),%eax
1961 if (!key || !pbData || !key->pProvider || key->pProvider->dwMagic !=
MAGIC_CRYPTPROV)

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #1 from Juan Lang   2010-02-01 14:11:41 ---
It's hard to diagnose this, because your backtrace doesn't have debug symbols
for the test files.  Still, I'll take a guess that it's this one, in
test_incorrect_api_usage():

result = pCryptDestroyKey(hKey2);
    ok (result, "%d\n", GetLastError());

dwTemp = CRYPT_MODE_ECB;    
    result = pCryptSetKeyParam(hKey2, KP_MODE, (BYTE*)&dwTemp, sizeof(DWORD));
    ok (!result && GetLastError() == ERROR_INVALID_PARAMETER, "%d\n",
GetLastError());

That is, a destroyed key is than used.  The bad address is 0xfeeefeee, which
corresponds to the free filler (in dlls/ntdll/heap.c):
#define ARENA_FREE_FILLER      0xfeeefeee

Basically, I think this is a false positive.  An app was doing something bogus
in the first place, and we wrote a test that mimics its behavior.  I'm not sure
how to fix the tests to pass in this case.

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

Semen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

Austin Lund  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #2 from Austin Lund   2010-07-09 03:54:08 ---
Does this really need to be tested for?  MSDN does say that if you call
CryptDestroy you should not use the handle again.  Is there an application
which uses this functionality?

If so, then it's wine that needs fixing and not the tests.  Is there a way to
know if a pointer has been freed or not?

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #3 from Juan Lang   2010-07-09 11:57:13 ---
(In reply to comment #2)
> Does this really need to be tested for?  MSDN does say that if you call
> CryptDestroy you should not use the handle again.  Is there an application
> which uses this functionality?

Yes.  The test code I already pointed out says which app.

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #4 from Austin Lund   2010-07-09 22:46:40 ---
OK. So wine needs to be fixed.  Is there a way to know if a pointer has been
freed?  Just looking through MSDN it seems there isn't.  If that's the case
then a list of valid handles will need to be kept to avoid referencing freed
memory.  Unless there is another option.

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #5 from Juan Lang   2010-07-12 11:56:37 ---
(In reply to comment #4)
> OK. So wine needs to be fixed.  Is there a way to know if a pointer has been
> freed?  Just looking through MSDN it seems there isn't.  If that's the case
> then a list of valid handles will need to be kept to avoid referencing freed
> memory.  Unless there is another option.

Wine doesn't need to be fixed, the tests need to be fixed not to crash when
+heap is enabled.  The app itself is buggy, there's no guarantee it wouldn't
crash with a different heap implementation under Windows.

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #6 from Austin Lund   2010-07-12 21:44:27 ---
(In reply to comment #5)
> (In reply to comment #4)
> > OK. So wine needs to be fixed.  Is there a way to know if a pointer has been
> > freed?  Just looking through MSDN it seems there isn't.  If that's the case
> > then a list of valid handles will need to be kept to avoid referencing freed
> > memory.  Unless there is another option.
>
> Wine doesn't need to be fixed, the tests need to be fixed not to crash when
> +heap is enabled.  The app itself is buggy, there's no guarantee it wouldn't
> crash with a different heap implementation under Windows.

That doesn't make sense to me.  Under the current test windows doesn't crash
and an app depends on that.  Wine has the possibility of crashing and does with
+heap.  So either the test is removed, or wine is changed to not crash like
windows.  I don't understand how the test can be fixed without being removed
whilst still testing the required behaviour.

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #7 from Andrew Nguyen   2010-07-13 03:33:13 ---
It seems reasonable to believe that an HCRYPTKEY handle that CryptGenKey
outputs is not a direct pointer to a data structure. Given that it's typedefed
as a ULONG_PTR, and doing something (admittedly crude) like:

HCRYPTKEY p;
BOOL result;
for (p = 0; p < 0x00400000; p++)
{
    result = pCryptDestroyKey(p);
    ok (!result && GetLastError() == ERROR_INVALID_PARAMETER, "%d\n",
GetLastError());
}

does not induce a crash, nor is there any evidence of the process heap being
corrupted (HeapValidate), this seems to suggest that some sort of handle table
should be employed.

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #8 from Austin Lund   2010-08-05 03:46:58 ---
Created an attachment (id=30013)
 --> (http://bugs.winehq.org/attachment.cgi?id=30013)
Adding handle table for HCRYPTKEY

I've tried to add a handle table implementation.  I've only done it for
HCRYPTKEY.  It does make that particular function call not crash in the test,
but then it crashes on a released HCRYPTHASH.  So if this is the "right"
approach, then it needs to be duplicated for the other handles.

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #9 from Austin Lund   2010-08-05 05:01:30 ---
Created an attachment (id=30014)
 --> (http://bugs.winehq.org/attachment.cgi?id=30014)
An alternative fix using SEH

It was suggested that the handle table solution is ugly and a better way would
be with SEH.  Another positive is that all of the crashes are solved with this
patch.

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #10 from Austin Lund   2010-08-17 20:43:21 ---
The suggestion on IRC was to use a magic code.  The patch for this is here:

http://www.winehq.org/pipermail/wine-patches/2010-August/092123.html

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

--- Comment #11 from Austin Lund   2010-08-18 17:40:31 ---
The magic values patch was committed: c98e6c09ae0073a1d73f638d6ee631984c0f528f

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #12 from Austin English   2010-08-18 17:50:07 ---
(In reply to comment #11)
> The magic values patch was committed: c98e6c09ae0073a1d73f638d6ee631984c0f528f

And today, the test passed:
http://test.winehq.org/data/83335590369da812440ecbda1c88ead24f1ec7dd/wine_ae...

thanks!

[Bug 21568] advapi32/crypt tests crash with +heap

http://bugs.winehq.org/show_bug.cgi?id=21568

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #13 from Alexandre Julliard   2010-08-20 12:39:14 ---
Closing bugs fixed in 1.3.1.