mshtml: Always set the URL policy in the error cases?

[Francois Gouget]

gcc 4.7 is complaining that policy may be used uninitialized in 
InternetHostSecurityManager_QueryCustomPolicy() and indeed there are 
many error cases in confirm_safety_load() and confirm_safety() where the 
policy is not set. The patch belowe seems to plug all these holes but I 
don't know if it really makes sense.

diff --git a/dlls/mshtml/secmgr.c b/dlls/mshtml/secmgr.c
index ff349ac..3f64a42 100644
--- a/dlls/mshtml/secmgr.c
+++ b/dlls/mshtml/secmgr.c
@@ -104,10 +104,9 @@ static HRESULT confirm_safety_load(HTMLDocumentNode *This, struct CONFIRMSAFETY
         CATID init_catid = CATID_SafeForInitializing;

hres = ICatInformation_IsClassOfCategories(This->catmgr, &cs->clsid, 1, &init_catid, 0, NULL);
+        *ret = hres == S_OK ? URLPOLICY_ALLOW : URLPOLICY_DISALLOW;
         if(FAILED(hres))
             return hres;
-
-        *ret = hres == S_OK ? URLPOLICY_ALLOW : URLPOLICY_DISALLOW;
     }

return S_OK;
@@ -157,13 +156,17 @@ static HRESULT confirm_safety(HTMLDocumentNode *This, const WCHAR *url, struct C
         if(!This->catmgr) {
             hres = CoCreateInstance(&CLSID_StdComponentCategoriesMgr, NULL, CLSCTX_INPROC_SERVER,
                     &IID_ICatInformation, (void**)&This->catmgr);
-            if(FAILED(hres))
+            if(FAILED(hres)) {
+                *ret = URLPOLICY_DISALLOW;
                 return hres;
+            }
         }

hres = ICatInformation_IsClassOfCategories(This->catmgr, &cs->clsid, 1, &scripting_catid, 0, NULL);
-        if(FAILED(hres))
+        if(FAILED(hres)) {
+            *ret = URLPOLICY_DISALLOW;
             return hres;
+        }

if(hres != S_OK) {
             *ret = URLPOLICY_DISALLOW;

[Jacek Caban]

Hi Francois,

On 06/28/12 16:27, Francois Gouget wrote:
> gcc 4.7 is complaining that policy may be used uninitialized in 
> InternetHostSecurityManager_QueryCustomPolicy() and indeed there are 
> many error cases in confirm_safety_load() and confirm_safety() where the 
> policy is not set. The patch belowe seems to plug all these holes but I 
> don't know if it really makes sense.

Sorry for the delay. I recall that exactly this issue has already popped
out. I think that the current code is fine and there is nothing to fix.
However, given that it happens in already released stable GCC, it seems
like after all we have to do something about it. These are mostly
theoretical code paths, so the way it's fixed doesn't really matter
much. It would probably be more appropriate to add
assert(SUCCEEDED(hres)); after IsClassOfCategories calls, which can't fail.

Jacek

[Francois Gouget]

On Tue, 3 Jul 2012, Jacek Caban wrote:
[...]
> It would probably be more appropriate to add
> assert(SUCCEEDED(hres)); after IsClassOfCategories calls, which can't fail.

Ok. Would something like this be ok? It seems like this single assert() 
is enough to make gcc happy here.

commit 2c47407d6e11ddb66050890abd5403b302c7d6c6
Author: Francois Gouget 
Date:   Mon Jul 16 10:33:50 2012 +0200

mshtml: Avoid returning with an unset URL policy in error cases.

diff --git a/dlls/mshtml/secmgr.c b/dlls/mshtml/secmgr.c
index ff349ac..f961bca 100644
--- a/dlls/mshtml/secmgr.c
+++ b/dlls/mshtml/secmgr.c
@@ -20,6 +20,7 @@

#include <stdarg.h>
 #include <stdio.h>
+#include <assert.h>

#define COBJMACROS

@@ -104,9 +105,7 @@ static HRESULT confirm_safety_load(HTMLDocumentNode *This, struct CONFIRMSAFETY
         CATID init_catid = CATID_SafeForInitializing;

hres = ICatInformation_IsClassOfCategories(This->catmgr, &cs->clsid, 1, &init_catid, 0, NULL);
-        if(FAILED(hres))
-            return hres;
-
+        assert(SUCCEEDED(hres));
         *ret = hres == S_OK ? URLPOLICY_ALLOW : URLPOLICY_DISALLOW;
     }

[Jacek Caban]

On 07/16/12 10:37, Francois Gouget wrote:
> On Tue, 3 Jul 2012, Jacek Caban wrote:
> [...]
> > It would probably be more appropriate to add
> > assert(SUCCEEDED(hres)); after IsClassOfCategories calls, which can't fail.
>
> Ok. Would something like this be ok? It seems like this single assert() 
> is enough to make gcc happy here.

Looks good to me.

Thanks,
Jacek