[PATCH 1/3] wintrust: Refactor CryptCATOpen error handling.
Make it simpler, handle all errors in the same place. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/wintrust/crypt.c | 118 +++++++++++++----------------------------- 1 file changed, 36 insertions(+), 82 deletions(-) diff --git a/dlls/wintrust/crypt.c b/dlls/wintrust/crypt.c index be869ad8277..4fbf75f7e0c 100644 --- a/dlls/wintrust/crypt.c +++ b/dlls/wintrust/crypt.c @@ -904,10 +904,10 @@ BOOL WINAPI CryptCATPersistStore(HANDLE catalog) HANDLE WINAPI CryptCATOpen(WCHAR *filename, DWORD flags, HCRYPTPROV hProv, DWORD dwPublicVersion, DWORD dwEncodingType) { - HANDLE file, hmsg; - BYTE *buffer = NULL; - DWORD size, open_mode = OPEN_ALWAYS; - struct cryptcat *cc; + HANDLE file, hmsg = NULL; + BYTE *buffer = NULL, *p; + DWORD i, sum = 0, size, open_mode = OPEN_ALWAYS; + struct cryptcat *cc = NULL; TRACE("filename %s, flags %#x, provider %#lx, version %#x, type %#x\n", debugstr_w(filename), flags, hProv, dwPublicVersion, dwEncodingType); @@ -929,92 +929,46 @@ HANDLE WINAPI CryptCATOpen(WCHAR *filename, DWORD flags, HCRYPTPROV hProv, if (file == INVALID_HANDLE_VALUE) return INVALID_HANDLE_VALUE; size = GetFileSize(file, NULL); - if (!(buffer = HeapAlloc(GetProcessHeap(), 0, size))) - { - CloseHandle(file); - SetLastError(ERROR_OUTOFMEMORY); - return INVALID_HANDLE_VALUE; - } - if (!(hmsg = CryptMsgOpenToDecode(dwEncodingType, 0, 0, hProv, NULL, NULL))) - { - CloseHandle(file); - HeapFree(GetProcessHeap(), 0, buffer); - return INVALID_HANDLE_VALUE; - } - if (!ReadFile(file, buffer, size, &size, NULL) || !CryptMsgUpdate(hmsg, buffer, size, TRUE)) - { - CloseHandle(file); - HeapFree(GetProcessHeap(), 0, buffer); - CryptMsgClose(hmsg); - return INVALID_HANDLE_VALUE; - } - HeapFree(GetProcessHeap(), 0, buffer); - CloseHandle(file); + if (!(buffer = HeapAlloc(GetProcessHeap(), 0, size))) goto failed_alloc; + if (!(hmsg = CryptMsgOpenToDecode(dwEncodingType, 0, 0, hProv, NULL, NULL))) goto failed; + if (!ReadFile(file, buffer, size, &size, NULL) || !CryptMsgUpdate(hmsg, buffer, size, TRUE)) goto failed; size = sizeof(DWORD); - if (!(cc = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*cc)))) - { - CryptMsgClose(hmsg); - SetLastError(ERROR_OUTOFMEMORY); - return INVALID_HANDLE_VALUE; - } + if (!(cc = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*cc)))) goto failed_alloc; cc->msg = hmsg; cc->encoding = dwEncodingType; - if (CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_COUNT_PARAM, 0, &cc->attr_count, &size)) - { - DWORD i, sum = 0; - BYTE *p; + if (!CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_COUNT_PARAM, 0, &cc->attr_count, &size)) goto failed; - for (i = 0; i < cc->attr_count; i++) - { - if (!CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_PARAM, i, NULL, &size)) - { - CryptMsgClose(hmsg); - HeapFree(GetProcessHeap(), 0, cc); - return INVALID_HANDLE_VALUE; - } - sum += size; - } - if (!(cc->attr = HeapAlloc(GetProcessHeap(), 0, sizeof(*cc->attr) * cc->attr_count + sum))) - { - CryptMsgClose(hmsg); - HeapFree(GetProcessHeap(), 0, cc); - SetLastError(ERROR_OUTOFMEMORY); - return INVALID_HANDLE_VALUE; - } - p = (BYTE *)(cc->attr + cc->attr_count); - for (i = 0; i < cc->attr_count; i++) - { - if (!CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_PARAM, i, NULL, &size)) - { - CryptMsgClose(hmsg); - HeapFree(GetProcessHeap(), 0, cc->attr); - HeapFree(GetProcessHeap(), 0, cc); - return INVALID_HANDLE_VALUE; - } - if (!CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_PARAM, i, p, &size)) - { - CryptMsgClose(hmsg); - HeapFree(GetProcessHeap(), 0, cc->attr); - HeapFree(GetProcessHeap(), 0, cc); - return INVALID_HANDLE_VALUE; - } - p += size; - } - cc->inner = decode_inner_content(hmsg, dwEncodingType, &cc->inner_len); - if (!cc->inner || !CryptSIPRetrieveSubjectGuid(filename, NULL, &cc->subject)) - { - CryptMsgClose(hmsg); - HeapFree(GetProcessHeap(), 0, cc->attr); - HeapFree(GetProcessHeap(), 0, cc->inner); - HeapFree(GetProcessHeap(), 0, cc); - return INVALID_HANDLE_VALUE; - } - cc->magic = CRYPTCAT_MAGIC; - return cc; + for (i = 0; i < cc->attr_count; i++) + { + if (!CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_PARAM, i, NULL, &size)) goto failed; + sum += size; + } + if (!(cc->attr = HeapAlloc(GetProcessHeap(), 0, sizeof(*cc->attr) * cc->attr_count + sum))) goto failed_alloc; + p = (BYTE *)(cc->attr + cc->attr_count); + for (i = 0; i < cc->attr_count; i++) + { + if (!CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_PARAM, i, NULL, &size)) goto failed; + if (!CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_PARAM, i, p, &size)) goto failed; + p += size; } + cc->inner = decode_inner_content(hmsg, dwEncodingType, &cc->inner_len); + if (!cc->inner || !CryptSIPRetrieveSubjectGuid(filename, NULL, &cc->subject)) goto failed; + cc->magic = CRYPTCAT_MAGIC; + HeapFree(GetProcessHeap(), 0, buffer); + CloseHandle(file); + return cc; + +failed_alloc: + SetLastError(ERROR_OUTOFMEMORY); +failed: + if (cc) HeapFree(GetProcessHeap(), 0, cc->inner); + if (cc) HeapFree(GetProcessHeap(), 0, cc->attr); HeapFree(GetProcessHeap(), 0, cc); + HeapFree(GetProcessHeap(), 0, buffer); + CryptMsgClose(hmsg); + CloseHandle(file); return INVALID_HANDLE_VALUE; } -- 2.29.2
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/wintrust/crypt.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dlls/wintrust/crypt.c b/dlls/wintrust/crypt.c index 4fbf75f7e0c..e9a58a84ac4 100644 --- a/dlls/wintrust/crypt.c +++ b/dlls/wintrust/crypt.c @@ -928,16 +928,16 @@ HANDLE WINAPI CryptCATOpen(WCHAR *filename, DWORD flags, HCRYPTPROV hProv, file = CreateFileW(filename, GENERIC_READ, FILE_SHARE_READ, NULL, open_mode, 0, NULL); if (file == INVALID_HANDLE_VALUE) return INVALID_HANDLE_VALUE; + if (!(hmsg = CryptMsgOpenToDecode(dwEncodingType, 0, 0, hProv, NULL, NULL))) goto failed; + if (!(cc = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*cc)))) goto failed_alloc; + cc->msg = hmsg; + cc->encoding = dwEncodingType; + size = GetFileSize(file, NULL); if (!(buffer = HeapAlloc(GetProcessHeap(), 0, size))) goto failed_alloc; - if (!(hmsg = CryptMsgOpenToDecode(dwEncodingType, 0, 0, hProv, NULL, NULL))) goto failed; if (!ReadFile(file, buffer, size, &size, NULL) || !CryptMsgUpdate(hmsg, buffer, size, TRUE)) goto failed; size = sizeof(DWORD); - if (!(cc = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*cc)))) goto failed_alloc; - - cc->msg = hmsg; - cc->encoding = dwEncodingType; if (!CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_COUNT_PARAM, 0, &cc->attr_count, &size)) goto failed; for (i = 0; i < cc->attr_count; i++) -- 2.29.2
Fixes a regression from 699e0a55ea71e2506917e38fc85cb4ae23a9cd1a, making "winetricks dotnet30sp1" fail. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49831 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- Note that sometimes (often?) "winetricks dotnet30sp1" still fails early because NetFx20SP1_x86.exe fails to install. It's unrelated, and happens spuriously. dlls/wintrust/crypt.c | 5 ++++- dlls/wintrust/tests/crypt.c | 12 ++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/dlls/wintrust/crypt.c b/dlls/wintrust/crypt.c index e9a58a84ac4..2a780e57546 100644 --- a/dlls/wintrust/crypt.c +++ b/dlls/wintrust/crypt.c @@ -935,7 +935,7 @@ HANDLE WINAPI CryptCATOpen(WCHAR *filename, DWORD flags, HCRYPTPROV hProv, size = GetFileSize(file, NULL); if (!(buffer = HeapAlloc(GetProcessHeap(), 0, size))) goto failed_alloc; - if (!ReadFile(file, buffer, size, &size, NULL) || !CryptMsgUpdate(hmsg, buffer, size, TRUE)) goto failed; + if (!ReadFile(file, buffer, size, &size, NULL) || !CryptMsgUpdate(hmsg, buffer, size, TRUE)) goto done; size = sizeof(DWORD); if (!CryptMsgGetParam(hmsg, CMSG_ATTR_CERT_COUNT_PARAM, 0, &cc->attr_count, &size)) goto failed; @@ -955,9 +955,12 @@ HANDLE WINAPI CryptCATOpen(WCHAR *filename, DWORD flags, HCRYPTPROV hProv, } cc->inner = decode_inner_content(hmsg, dwEncodingType, &cc->inner_len); if (!cc->inner || !CryptSIPRetrieveSubjectGuid(filename, NULL, &cc->subject)) goto failed; + +done: cc->magic = CRYPTCAT_MAGIC; HeapFree(GetProcessHeap(), 0, buffer); CloseHandle(file); + SetLastError(ERROR_SUCCESS); return cc; failed_alloc: diff --git a/dlls/wintrust/tests/crypt.c b/dlls/wintrust/tests/crypt.c index 1b436e9f4ab..5f9c402e00f 100644 --- a/dlls/wintrust/tests/crypt.c +++ b/dlls/wintrust/tests/crypt.c @@ -429,10 +429,10 @@ static void test_CryptCATOpen(void) } else { - todo_wine ok(cat != INVALID_HANDLE_VALUE, "flags %#x: expected success\n", flags); - todo_wine ok(!GetLastError(), "flags %#x: got error %u\n", flags, GetLastError()); + ok(cat != INVALID_HANDLE_VALUE, "flags %#x: expected success\n", flags); + ok(!GetLastError(), "flags %#x: got error %u\n", flags, GetLastError()); ret = pCryptCATClose(cat); - todo_wine ok(ret, "flags %#x: failed to close file\n", flags); + ok(ret, "flags %#x: failed to close file\n", flags); ret = DeleteFileW(filename); ok(ret, "flags %#x: failed to delete file, error %u\n", flags, GetLastError()); } @@ -443,10 +443,10 @@ static void test_CryptCATOpen(void) SetLastError(0xdeadbeef); cat = pCryptCATOpen(filename, flags, 0, 0, 0); - todo_wine ok(cat != INVALID_HANDLE_VALUE, "flags %#x: expected success\n", flags); - todo_wine ok(!GetLastError(), "flags %#x: got error %u\n", flags, GetLastError()); + ok(cat != INVALID_HANDLE_VALUE, "flags %#x: expected success\n", flags); + ok(!GetLastError(), "flags %#x: got error %u\n", flags, GetLastError()); ret = pCryptCATClose(cat); - todo_wine ok(ret, "flags %#x: failed to close file\n", flags); + ok(ret, "flags %#x: failed to close file\n", flags); file = _wfopen(filename, L"r"); ret = fread(buffer, 1, sizeof(buffer), file); -- 2.29.2
Rémi Bernon <rbernon(a)codeweavers.com> writes:
Make it simpler, handle all errors in the same place.
It would be better to leave that kind of refactoring for after code freeze. Any chance you could do more targeted fix just for that bug in the meantime? -- Alexandre Julliard julliard(a)winehq.org
On 12/16/20 9:13 PM, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
Make it simpler, handle all errors in the same place.
It would be better to leave that kind of refactoring for after code freeze. Any chance you could do more targeted fix just for that bug in the meantime?
I was afraid it would be the case, I'll send another version, with a simple diff, but that doesn't make the code simpler. -- Rémi Bernon <rbernon(a)codeweavers.com>
participants (2)
-
Alexandre Julliard -
Rémi Bernon