Last Comment Bug 710995 - Possible bad null-check in ShortcutResolver::Init()
: Possible bad null-check in ShortcutResolver::Init()
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla11
Assigned To: Jim Mathies [:jimm]
: Nathan Froyd [:froydnj]
Depends on:
Blocks: 710966
  Show dependency treegraph
Reported: 2011-12-14 23:52 PST by Justin Dolske [:Dolske]
Modified: 2011-12-17 09:08 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (3.76 KB, patch)
2011-12-15 03:05 PST, Jim Mathies [:jimm]
robert.strong.bugs: review+
Details | Diff | Splinter Review

Description User image Justin Dolske [:Dolske] 2011-12-14 23:52:20 PST
18th section in
V595 The 'mShellLink' pointer was utilized before it was verified against nullptr. Check lines: 183, 187.
nslocalfilewin.cpp 183

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

    HRESULT hres; 
    hres = CoCreateInstance(CLSID_ShellLink,
    if (SUCCEEDED(hres))
        // Get a pointer to the IPersistFile interface.
        hres = mShellLink->QueryInterface(IID_IPersistFile,

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

    return NS_OK;
Comment 1 User image Jim Mathies [:jimm] 2011-12-15 03:05:27 PST
Created attachment 581926 [details] [diff] [review]

pushed to try just to be safe.
Comment 2 User image Jim Mathies [:jimm] 2011-12-15 05:45:33 PST
Comment on attachment 581926 [details] [diff] [review]

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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2011-12-15 16:10:09 PST
Comment on attachment 581926 [details] [diff] [review]

>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 5 User image Matt Brubeck (:mbrubeck) 2011-12-17 09:08:05 PST

Note You need to log in before you can comment on or make changes to this bug.