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

RESOLVED FIXED in mozilla11

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Dolske, Assigned: jimm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [pvs-studio])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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;
}
(Reporter)

Updated

6 years ago
Blocks: 710966
(Assignee)

Updated

6 years ago
Assignee: nobody → jmathies
(Assignee)

Comment 1

6 years ago
Created attachment 581926 [details] [diff] [review]
patch

pushed to try just to be safe.
(Assignee)

Comment 2

6 years ago
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.
Attachment #581926 - Flags: review?(robert.bugzilla)
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;
Attachment #581926 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d241a415d65a
https://hg.mozilla.org/mozilla-central/rev/d241a415d65a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.