[PATCH v7 0/1] MR3410: advpack: Add nullcheck for parameter that can be null (Coverity)
-- v7: advpack: Add nullchecks for parameters that can be null (Coverity) https://gitlab.winehq.org/wine/wine/-/merge_requests/3410
From: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/advpack/install.c | 7 +++++-- dlls/advpack/tests/install.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/dlls/advpack/install.c b/dlls/advpack/install.c index 81ceebbe7aa..b9293f6ac19 100644 --- a/dlls/advpack/install.c +++ b/dlls/advpack/install.c @@ -582,7 +582,7 @@ HRESULT WINAPI ExecuteCabA(HWND hwnd, CABINFOA* pCab, LPVOID pReserved) TRACE("(%p, %p, %p)\n", hwnd, pCab, pReserved); - if (!pCab) + if (!pCab || !pCab->pszInf) return E_INVALIDARG; if (pCab->pszCab) @@ -634,9 +634,12 @@ HRESULT WINAPI ExecuteCabW(HWND hwnd, CABINFOW* pCab, LPVOID pReserved) TRACE("(%p, %p, %p)\n", hwnd, pCab, pReserved); + if (!pCab || !pCab->pszInf || !*pCab->pszInf) + return E_INVALIDARG; + ZeroMemory(&info, sizeof(ADVInfo)); - if ((pCab->pszCab && *pCab->pszCab) && (pCab->pszInf && *pCab->pszInf) && *pCab->szSrcPath) + if ((pCab->pszCab && *pCab->pszCab) && *pCab->szSrcPath) { TRACE("pszCab: %s, pszInf: %s, szSrcPath: %s\n", debugstr_w(pCab->pszCab), debugstr_w(pCab->pszInf), debugstr_w(pCab->szSrcPath)); diff --git a/dlls/advpack/tests/install.c b/dlls/advpack/tests/install.c index 7d747b1f018..41cb7e9fbad 100644 --- a/dlls/advpack/tests/install.c +++ b/dlls/advpack/tests/install.c @@ -357,6 +357,38 @@ static void test_RegisterOCXs(void) DeleteFileA("test.inf"); } +static void test_ExecuteCab(void) +{ + HRESULT hr; + WCHAR wstr_empty[1] = {0}; + char str_empty[1] = {0}; + CABINFOA cabinfoa = {0}; + CABINFOW cabinfow = {0}; + + /* ExecuteCabA */ + + /* ExecuteCabA(NULL, NULL, NULL); Crashes on windows */ + + hr = ExecuteCabA(NULL, &cabinfoa, NULL); + ok(hr == E_INVALIDARG, "Got %lx\n", hr); + + cabinfoa.pszInf = str_empty; + hr = ExecuteCabA(NULL, &cabinfoa, NULL); + ok(hr == E_INVALIDARG, "Got %lx\n", hr); + + /* ExecuteCabW */ + + hr = ExecuteCabW(NULL, NULL, NULL); + ok(hr == E_INVALIDARG, "Got %lx\n", hr); + + hr = ExecuteCabW(NULL, &cabinfow, NULL); + ok(hr == E_INVALIDARG, "Got %lx\n", hr); + + cabinfow.pszInf = wstr_empty; + hr = ExecuteCabW(NULL, &cabinfow, NULL); + ok(hr == E_INVALIDARG, "Got %lx\n", hr); +} + START_TEST(install) { DWORD len; @@ -385,6 +417,7 @@ START_TEST(install) test_LaunchINFSection(); test_LaunchINFSectionEx(); test_RegisterOCXs(); + test_ExecuteCab(); FreeLibrary(hAdvPack); SetCurrentDirectoryA(prev_path); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3410
In general we don't add NULL checks unless there's an app that depends on them. I'm also not sure why the analyzer would complain about this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3410#note_121080
On Thu Nov 6 22:12:02 2025 +0000, Alexandre Julliard wrote:
In general we don't add NULL checks unless there's an app that depends on them. I'm also not sure why the analyzer would complain about this. That's because `ExecuteCabW` has the following code:
if ((pCab->pszCab && *pCab->pszCab) && (pCab->pszInf && *pCab->pszInf) && *pCab->szSrcPath)
{
// SNIP
}
hr = install_init(pCab->pszInf, pCab->pszSection, pCab->szSrcPath, pCab->dwFlags, &info);
Which implies that `pCab->pszInf` can be NULL, but it's dereferenced in `install_init` later. I thought I'd correct that mistake, but if you think it's fine as is, feel free to close this - there's currently no program I know of affected by it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3410#note_121110
On Thu Nov 6 22:12:02 2025 +0000, Fabian Maurer wrote:
That's because `ExecuteCabW` has the following code: ``` if ((pCab->pszCab && *pCab->pszCab) && (pCab->pszInf && *pCab->pszInf) && *pCab->szSrcPath) { // SNIP } hr = install_init(pCab->pszInf, pCab->pszSection, pCab->szSrcPath, pCab->dwFlags, &info); ``` Which implies that `pCab->pszInf` can be NULL, but it's dereferenced in `install_init` later. I thought I'd correct that mistake, but if you think it's fine as is, feel free to close this - there's currently no program I know of affected by it. If it's dereferenced anyway, and there's no app that crashes on this, it means that the NULL check is not necessary and can be removed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3410#note_121138
On Fri Nov 7 08:38:07 2025 +0000, Alexandre Julliard wrote:
If it's dereferenced anyway, and there's no app that crashes on this, it means that the NULL check is not necessary and can be removed. I guess? Or we could just do it properly and add the nullcheck
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3410#note_121168
On Fri Nov 7 14:51:48 2025 +0000, Fabian Maurer wrote:
I guess? Or we could just do it properly and add the nullcheck Then you would have to add more tests to determine the proper behavior, your tests are passing all nulls so they don't tell us anything about this specific pointer.
That seems like a lot of unnecessary work for something that no known app requires. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3410#note_121173
On Fri Nov 7 15:18:08 2025 +0000, Alexandre Julliard wrote:
Then you would have to add more tests to determine the proper behavior, your tests are passing all nulls so they don't tell us anything about this specific pointer. That seems like a lot of unnecessary work for something that no known app requires. Fair point. So should I remove the null pointer-check or leave it as-is?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3410#note_121246
participants (3)
-
Alexandre Julliard (＠julliard) -
Fabian Maurer
-
Fabian Maurer (＠DarkShadow44)