Re: [PATCH] shell32: handle bad menu handle (Coverity)

[Nikolay Sivov]

On 1/9/2010 16:59, Marcus Meissner wrote:
> Hi,
>
> handle "invalid menu" return -1. Now with test.
> Ciao, Marcus
>
> dlls/shell32/shlmenu.c         |    5 +++--
>   dlls/shell32/tests/shlfileop.c |   13 +++++++++++++
>   2 files changed, 16 insertions(+), 2 deletions(-)
> {
> diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c
> index 0a5cdca..4d74b93 100644
> --- a/dlls/shell32/tests/shlfileop.c
> +++ b/dlls/shell32/tests/shlfileop.c

Wrong file to place test in I think.
> @@ -2224,6 +2224,17 @@ static void test_unicode(void)
>       ok(!file_existsW(UNICODE_PATH), "The directory should have been removed\n");
>   }
>
> +extern HRESULT WINAPI Shell_MergeMenus (HMENU hmDst, HMENU hmSrc, UINT uInsert, UINT uIDAdjust, UINT uIDAdjustMax, ULONG uFlags);
> -

This should got to shlobj.h.
> +static void
> +test_shlmenu(void) {
> - HRESULT hres;
> - hres = Shell_MergeMenus (0, 0, 0x42, 0x4242, 0x424242, 0);
> - ok (hres == 0x4242, "expected 0x4242 but got %x\n", hres);
> - hres = Shell_MergeMenus ((HMENU)42, 0, 0x42, 0x4242, 0x424242, 0);
> - ok (hres == 0x4242, "expected 0x4242 but got %x\n", hres);
>
> +}
> -

Use a valid empty menu handle here as first parameter. Also I'd like to 
use more sane const values.
> START_TEST(shlfileop)
>   {
>       InitFunctionPointers();
> @@ -2263,4 +2274,6 @@ START_TEST(shlfileop)
>       clean_after_shfo_tests();
>   test_unicode();
> -
> - test_shlmenu();
> }

Function is available starting XP SP2 per msdn, so dynamic load should 
be used.

[PATCH] shell32: handle bad menu handle (Coverity)

[Marcus Meissner]

On Sat, Jan 09, 2010 at 07:18:18PM +0300, Nikolay Sivov wrote:
> On 1/9/2010 16:59, Marcus Meissner wrote:
> > Hi,
> >
> > handle "invalid menu" return -1. Now with test.
> > Ciao, Marcus
> >
> > dlls/shell32/shlmenu.c         |    5 +++--
> >  dlls/shell32/tests/shlfileop.c |   13 +++++++++++++
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > {
> > diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c
> > index 0a5cdca..4d74b93 100644
> > --- a/dlls/shell32/tests/shlfileop.c
> > +++ b/dlls/shell32/tests/shlfileop.c
>
> Wrong file to place test in I think.
> > @@ -2224,6 +2224,17 @@ static void test_unicode(void)
> >      ok(!file_existsW(UNICODE_PATH), "The directory should have been removed\n");
> >  }
> >
> > +extern HRESULT WINAPI Shell_MergeMenus (HMENU hmDst, HMENU hmSrc, UINT uInsert, UINT uIDAdjust, UINT uIDAdjustMax, ULONG uFlags);
> > -
>
> This should got to shlobj.h.
> > +static void
> > +test_shlmenu(void) {
> > - HRESULT hres;
> > - hres = Shell_MergeMenus (0, 0, 0x42, 0x4242, 0x424242, 0);
> > - ok (hres == 0x4242, "expected 0x4242 but got %x\n", hres);
> > - hres = Shell_MergeMenus ((HMENU)42, 0, 0x42, 0x4242, 0x424242, 0);
> > - ok (hres == 0x4242, "expected 0x4242 but got %x\n", hres);
> >
> > +}
> > -
>
> Use a valid empty menu handle here as first parameter. Also I'd like
> to use more sane const values.

Hm. What are valid empty menu handles? 0xffff ?
Or create one? and more sane?
> > START_TEST(shlfileop)
> >  {
> >      InitFunctionPointers();
> > @@ -2263,4 +2274,6 @@ START_TEST(shlfileop)
> >      clean_after_shfo_tests();
> >  test_unicode();
> > -
> > - test_shlmenu();
> >
> > }
>
> Function is available starting XP SP2 per msdn, so dynamic load
> should be used.

Testbot had no complaints for any version it tests, see 
https://winetestbot.geldorp.nl/JobDetails.pl?Key=279,
but i can change it.

Ciao, Marcus

[PATCH] shell32: handle bad menu handle (Coverity)

[Nikolay Sivov]

On 1/9/2010 19:37, Marcus Meissner wrote:
> > > +static void
> > > +test_shlmenu(void) {
> > > - HRESULT hres;
> > > - hres = Shell_MergeMenus (0, 0, 0x42, 0x4242, 0x424242, 0);
> > > - ok (hres == 0x4242, "expected 0x4242 but got %x\n", hres);
> > > - hres = Shell_MergeMenus ((HMENU)42, 0, 0x42, 0x4242, 0x424242, 0);
> > > - ok (hres == 0x4242, "expected 0x4242 but got %x\n", hres);
> > >
> > > +}
> > > -
> >
> > Use a valid empty menu handle here as first parameter. Also I'd like
> > to use more sane const values.
>
> Hm. What are valid empty menu handles? 0xffff ?
> Or create one? and more sane?

I mean CreateMenu() - it creates valid handle to empty menu, you'll test 
for item count in call you are testing.
About const values I mean something like 1 will be enough here.
> > > START_TEST(shlfileop)
> > >   {
> > >       InitFunctionPointers();
> > > @@ -2263,4 +2274,6 @@ START_TEST(shlfileop)
> > >       clean_after_shfo_tests();
> > >   test_unicode();
> > > -
> > > - test_shlmenu();
> > > }
> >
> > Function is available starting XP SP2 per msdn, so dynamic load
> > should be used.
>
> Testbot had no complaints for any version it tests, see
> https://winetestbot.geldorp.nl/JobDetails.pl?Key=279,
> but i can change it.

Ok, still it doesn't have 9x VMs, it's better to try first. We could ask 
Paul maybe.
> Ciao, Marcus

[PATCH] shell32: handle bad menu handle (Coverity)

[Paul Vriens]

On 01/09/2010 11:50 PM, Nikolay Sivov wrote:
> On 1/9/2010 19:37, Marcus Meissner wrote:
> > > > +static void
> > > > +test_shlmenu(void) {
> > > > - HRESULT hres;
> > > > - hres = Shell_MergeMenus (0, 0, 0x42, 0x4242, 0x424242, 0);
> > > > - ok (hres == 0x4242, "expected 0x4242 but got %x\n", hres);
> > > > - hres = Shell_MergeMenus ((HMENU)42, 0, 0x42, 0x4242, 0x424242, 0);
> > > > - ok (hres == 0x4242, "expected 0x4242 but got %x\n", hres);
> > > >
> > > > +}
> > > > -
> > >
> > > Use a valid empty menu handle here as first parameter. Also I'd like
> > > to use more sane const values.
> >
> > Hm. What are valid empty menu handles? 0xffff ?
> > Or create one? and more sane?
>
> I mean CreateMenu() - it creates valid handle to empty menu, you'll test
> for item count in call you are testing.
> About const values I mean something like 1 will be enough here.
> > > > START_TEST(shlfileop)
> > > > {
> > > > InitFunctionPointers();
> > > > @@ -2263,4 +2274,6 @@ START_TEST(shlfileop)
> > > > clean_after_shfo_tests();
> > > >
> > > > test_unicode();
> > > > -
> > > > - test_shlmenu();
> > > >
> > > > }
> > >
> > > Function is available starting XP SP2 per msdn, so dynamic load
> > > should be used.

Are you sure? It reads to me as being available up to XP-SP2/W2K3 but 
maybe removed or changed in newer versions. Anyway, we shouldn't ever 
believe MSDN as even their remark about needed shell32 version 6 or 
higher is not correct (see below) ;)
> > Testbot had no complaints for any version it tests, see
> > https://winetestbot.geldorp.nl/JobDetails.pl?Key=279,
> > but i can change it.
>
> Ok, still it doesn't have 9x VMs, it's better to try first. We could ask
> Paul maybe.

Succeeds on Win95.

[PATCH] shell32: handle bad menu handle (Coverity)

[Nikolay Sivov]

On 1/10/2010 13:35, Paul Vriens wrote:
> Are you sure? It reads to me as being available up to XP-SP2/W2K3 but 
> maybe removed or changed in newer versions. Anyway, we shouldn't ever 
> believe MSDN as even their remark about needed shell32 version 6 or 
> higher is not correct (see below) ;)

I misread it maybe:
---
This function is available through Windows XP Service Pack 2 (SP2) and 
Windows Server 2003.
---
> > > Testbot had no complaints for any version it tests, see
> > > https://winetestbot.geldorp.nl/JobDetails.pl?Key=279,
> > > but i can change it.
> >
> > Ok, still it doesn't have 9x VMs, it's better to try first. We could ask
> > Paul maybe.
>
> Succeeds on Win95.

Ok.