710995 - Possible bad null-check in ShortcutResolver::Init()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Justin Dolske [:Dolske]]

From http://www.viva64.com/en/a/0078/,
18th section in http://www.viva64.com/external-pictures/txt/mozilla-test.txt
V595 The 'mShellLink' pointer was utilized before it was verified against nullptr. Check lines: 183, 187.
nslocalfilewin.cpp 183

nsresult
ShortcutResolver::Init()
{
    CoInitialize(NULL);  // FIX: we should probably move somewhere higher up during startup

    HRESULT hres; 
    hres = CoCreateInstance(CLSID_ShellLink,
                            NULL,
                            CLSCTX_INPROC_SERVER,
                            IID_IShellLinkW,
                            (void**)&(mShellLink));
    if (SUCCEEDED(hres))
    {
        // Get a pointer to the IPersistFile interface.
        hres = mShellLink->QueryInterface(IID_IPersistFile,
                                          (void**)&mPersistFile);
    }

    if (mPersistFile == nsnull || mShellLink == nsnull)
        return NS_ERROR_FAILURE;

    return NS_OK;
}

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

pushed to try just to be safe.

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 581926 [details] [diff] [review]
patch

This really isn't a valid bug since the |if (SUCCEEDED(hres))| check after create instance insured the shell link was valid. But I went ahead and cleaned this up anyway. This patch passed try fine.

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 581926 [details] [diff] [review]
patch

>diff --git a/xpcom/io/nsLocalFileWin.cpp b/xpcom/io/nsLocalFileWin.cpp
>--- a/xpcom/io/nsLocalFileWin.cpp
>+++ b/xpcom/io/nsLocalFileWin.cpp
>...
> // |out| must be an allocated buffer of size MAX_PATH
> nsresult
> ShortcutResolver::Resolve(const WCHAR* in, WCHAR* out)
> {
>+    if (!mShellLink)
>+        return NS_ERROR_FAILURE;
>+
>     MutexAutoLock lock(mLock);
> 
>     // see if we can Load the path.
nit: might as well update the comment to reflect all that happens though I think it could just as easily be removed since the calls are self-explanatory. If you'd like to keep it perhaps like so?
See if we can Load the path, resolve the link, and get the path to the link target.

>-    HRESULT hres = mPersistFile->Load(in, STGM_READ);
>-
>-    if (FAILED(hres))
>-        return NS_ERROR_FAILURE;
>-
>-    // Resolve the link.
>-    hres = mShellLink->Resolve(nsnull, SLR_NO_UI);
>-
>-    if (FAILED(hres))
>-        return NS_ERROR_FAILURE;
>-
>-    // Get the path to the link target.
>-    hres = mShellLink->GetPath(out, MAX_PATH, NULL, SLGP_UNCPRIORITY);
>-
>-    if (FAILED(hres))
>+    if (FAILED(mPersistFile->Load(in, STGM_READ)) ||
>+        FAILED(mShellLink->Resolve(nsnull, SLR_NO_UI)) ||
>+        FAILED(mShellLink->GetPath(out, MAX_PATH, NULL, SLGP_UNCPRIORITY)))
>         return NS_ERROR_FAILURE;
>     return NS_OK;

Comment 4

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d241a415d65a

Comment 5

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d241a415d65a