Re: [PATCH 2/2] wininet/tests: Added checks for bad return values and skip tests when it happens. (try 2)

[Alexandre Julliard]

Morten Rønne  writes:
> diff --git a/dlls/wininet/tests/urlcache.c b/dlls/wininet/tests/urlcache.c
> index 02d8d28..1bf3f23 100644
> --- a/dlls/wininet/tests/urlcache.c
> +++ b/dlls/wininet/tests/urlcache.c
> @@ -60,7 +60,20 @@ static void test_find_url_cache_entriesA(void)
>      hEnumHandle = FindFirstUrlCacheEntry(NULL, NULL, &cbCacheEntryInfo);
>      ok(!hEnumHandle, "FindFirstUrlCacheEntry should have failed\n");
>      ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, "FindFirstUrlCacheEntry should have set last error to ERROR_INSUFFICIENT_BUFFER instead of %d\n", GetLastError());
> - if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
> - {
> -
>    skip("Find*UrlCacheEntry tests skipped, due to wrong return value\n");
> -
>    return;
> - }

You don't need to add skip() for that sort of thing, it's not supposed
to happen, and if it does there will already be a test failure in the
report.

[PATCH 2/2] wininet/tests: Added checks for bad return values and skip tests when it happens. (try 2)

[Morten Rønne]

Hi Alexandre

Well if you do not get a ERROR_INSUFFICIENT_BUFFER back, then 
cbCacheEntryInfo will not be changed, and that means that using that 
value in a following HeapAlloc may cause different kind of problems.
If cbCacheEntryInfo were zero, this will cause the following HeapAlloc 
to succesed, and return a pointer to a zero size buffer.
The pointer will in turn be used to access some element outside of the 
zero size buffer.
This will either give a random value, which isn't a problem, or if the 
allocated block is on the border of allocated memory, this will actually 
cause a SIGSEGV. And I consider that to be a bad thing.
So I think you need to test for that value, if do not want "random" 
SIGSEGV to happen during tests.

That problem is a common problem when the urlcache run full. And 
although Piotr have added code that will expand the urlcache, there is a 
limit on the cache, so it may run full for some people.

Best Regards
Morten Rønne

Den 09-04-2012 18:22, Alexandre Julliard skrev:
> Morten Rønne  writes:
> > diff --git a/dlls/wininet/tests/urlcache.c b/dlls/wininet/tests/urlcache.c
> > index 02d8d28..1bf3f23 100644
> > --- a/dlls/wininet/tests/urlcache.c
> > +++ b/dlls/wininet/tests/urlcache.c
> > @@ -60,7 +60,20 @@ static void test_find_url_cache_entriesA(void)
> >       hEnumHandle = FindFirstUrlCacheEntry(NULL, NULL,&cbCacheEntryInfo);
> >       ok(!hEnumHandle, "FindFirstUrlCacheEntry should have failed\n");
> >       ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, "FindFirstUrlCacheEntry should have set last error to ERROR_INSUFFICIENT_BUFFER instead of %d\n", GetLastError());
> > - if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
> > - {
> > -
> >    skip("Find*UrlCacheEntry tests skipped, due to wrong return value\n");
> > -
> >    return;
> > - }
>
> You don't need to add skip() for that sort of thing, it's not supposed
> to happen, and if it does there will already be a test failure in the
> report.

[PATCH 2/2] wininet/tests: Added checks for bad return values and skip tests when it happens. (try 2)

[Alexandre Julliard]

Morten Rønne  writes:
> Hi Alexandre
>
> Well if you do not get a ERROR_INSUFFICIENT_BUFFER back, then
> cbCacheEntryInfo will not be changed, and that means that using that
> value in a following HeapAlloc may cause different kind of problems.
> If cbCacheEntryInfo were zero, this will cause the following HeapAlloc
> to succesed, and return a pointer to a zero size buffer.

Leaving the function at that point is fine, my point was that the skip()
is unnecessary.