Last Comment Bug 710995 - Possible bad null-check in ShortcutResolver::Init()
: Possible bad null-check in ShortcutResolver::Init()
Status: RESOLVED FIXED
[pvs-studio]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jim Mathies [:jimm]
:
: Nathan Froyd [:froydnj]
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description Justin Dolske [:Dolske] 2011-12-14 23:52:20 PST
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] 2011-12-15 03:05:27 PST
Created attachment 581926 [details] [diff] [review]
patch

pushed to try just to be safe.
Comment 2 Jim Mathies [:jimm] 2011-12-15 05:45:33 PST
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 [:rstrong] (use needinfo to contact me) 2011-12-15 16:10:09 PST
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 5 Matt Brubeck (:mbrubeck) 2011-12-17 09:08:05 PST
https://hg.mozilla.org/mozilla-central/rev/d241a415d65a

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