[PATCH 1/1] wininet/test: Check null pointer in InternetGetSecurityInfoByURLW

[Jactry Zeng]

Hi Wei,

Thanks for contribution!
I have a question about the tests:

+
+        SetLastError(0);
+        res = pInternetGetSecurityInfoByURLA(url, &chain, NULL);
+        ok_(__FILE__,line)(res && GetLastError() == 0,
+                           "InternetGetSecurityInfoByURLA failed returned:
%x(%u), expected success\n", res, GetLastError());
+
+        SetLastError(0);
+        res = pInternetGetSecurityInfoByURLA(url, NULL, &flags);
+        ok_(__FILE__,line)(res && GetLastError() == 0,
+                           "InternetGetSecurityInfoByURLA failed returned:
%x(%u), expected success\n", res, GetLastError());
     }else {

Why not set last error as 0xdeadbeef in these two tests?

2018-01-08 15:57 GMT+08:00 Wei Xie :
>

[Jacek Caban]

On 09.01.2018 09:28, Jactry Zeng wrote:
> Hi Wei,
>
> Thanks for contribution!
> I have a question about the tests:
> -
>
> +        SetLastError(0);
> +        res = pInternetGetSecurityInfoByURLA(url, &chain, NULL);
> +        ok_(__FILE__,line)(res && GetLastError() == 0,
> +                           "InternetGetSecurityInfoByURLA failed
> returned: %x(%u), expected success\n", res, GetLastError());
> -
>
> +        SetLastError(0);
> +        res = pInternetGetSecurityInfoByURLA(url, NULL, &flags);
> +        ok_(__FILE__,line)(res && GetLastError() == 0,
> +                           "InternetGetSecurityInfoByURLA failed
> returned: %x(%u), expected success\n", res, GetLastError());
>      }else {
>
> Why not set last error as 0xdeadbeef in these two tests?

Actually (with a few exceptions when apps really depend on it), it's
better to not test last error on success. Caller should never use it
anyway and it might be polluted by internal calls that we're not
interested in.

Wei, I sent a modified version of your patch. I hope you're fine with
those changes. Thanks for contribution.

Jacek

[PATCH 1/1] wininet/test: Check null pointer inInternetGetSecurityInfoByURLW

[Wei Xie]

Hi Jacek:

I don't received your modified patch.

CertFreeCertificateChain(chain);
+
+        res = pInternetGetSecurityInfoByURLA(url, &chain, NULL);
+        ok_(__FILE__,line)(res, "InternetGetSecurityInfoByURLA failed: %u\n", GetLastError());
+
+        res = pInternetGetSecurityInfoByURLA(url, NULL, &flags);
+        ok_(__FILE__,line)(res, "InternetGetSecurityInfoByURLA failed: %u\n", GetLastError());
+
+        SetLastError(0xdeadbeef);
+        res = pInternetGetSecurityInfoByURLA(url, NULL, NULL);
+        ok_(__FILE__,line)(!res && GetLastError() == ERROR_INVALID_PARAMETER,
+                           "InternetGetSecurityInfoByURLA returned: %x(%u), expected failed %u\n",
+                            res, GetLastError(), ERROR_INVALID_PARAMETER);
     }else {

This is my modified version, is this ok? Thanks for your help.

------------------ Original ------------------
From:  "Jacek Caban";
Date:  Tue, Jan 9, 2018 07:09 PM
To:  "Jactry Zeng"; "Wei Xie";
Cc:  "wine-devel";
Subject:  Re: [PATCH 1/1] wininet/test: Check null pointer inInternetGetSecurityInfoByURLW

On 09.01.2018 09:28, Jactry Zeng wrote:
Hi Wei,

Thanks for contribution!
I have a question about the tests:

+
+        SetLastError(0);
+        res = pInternetGetSecurityInfoByURLA(url, &chain, NULL);
+        ok_(__FILE__,line)(res && GetLastError() == 0,
+                           "InternetGetSecurityInfoByURLA failed returned: %x(%u), expected success\n", res, GetLastError());
+
+        SetLastError(0);
+        res = pInternetGetSecurityInfoByURLA(url, NULL, &flags);
+        ok_(__FILE__,line)(res && GetLastError() == 0,
+                           "InternetGetSecurityInfoByURLA failed returned: %x(%u), expected success\n", res, GetLastError());
     }else {

Why not set last error as 0xdeadbeef in these two tests?

Actually (with a few exceptions when apps really depend on it), it's better to not test last error on success. Caller should never use it anyway and it might be polluted by internal calls that we're not interested in.

Wei, I sent a modified version of your patch. I hope you're fine with those changes. Thanks for contribution.

Jacek

[PATCH 1/1] wininet/test: Check null pointer inInternetGetSecurityInfoByURLW

[Wei Xie]

Hi Jacek:

I found the patch is committed, Thanks

------------------ Original ------------------
From:  "Wei Xie";
Date:  Wed, Jan 10, 2018 09:41 AM
To:  "Jacek Caban"; "Jactry Zeng";
Cc:  "wine-devel";
Subject:  Re: [PATCH 1/1] wininet/test: Check null pointer inInternetGetSecurityInfoByURLW

Hi Jacek:

I don't received your modified patch.

CertFreeCertificateChain(chain);
+
+        res = pInternetGetSecurityInfoByURLA(url, &chain, NULL);
+        ok_(__FILE__,line)(res, "InternetGetSecurityInfoByURLA failed: %u\n", GetLastError());
+
+        res = pInternetGetSecurityInfoByURLA(url, NULL, &flags);
+        ok_(__FILE__,line)(res, "InternetGetSecurityInfoByURLA failed: %u\n", GetLastError());
+
+        SetLastError(0xdeadbeef);
+        res = pInternetGetSecurityInfoByURLA(url, NULL, NULL);
+        ok_(__FILE__,line)(!res && GetLastError() == ERROR_INVALID_PARAMETER,
+                           "InternetGetSecurityInfoByURLA returned: %x(%u), expected failed %u\n",
+                            res, GetLastError(), ERROR_INVALID_PARAMETER);
     }else {

This is my modified version, is this ok? Thanks for your help.

------------------ Original ------------------
From:  "Jacek Caban";
Date:  Tue, Jan 9, 2018 07:09 PM
To:  "Jactry Zeng"; "Wei Xie";
Cc:  "wine-devel";
Subject:  Re: [PATCH 1/1] wininet/test: Check null pointer inInternetGetSecurityInfoByURLW

On 09.01.2018 09:28, Jactry Zeng wrote:
Hi Wei,

Thanks for contribution!
I have a question about the tests:

+
+        SetLastError(0);
+        res = pInternetGetSecurityInfoByURLA(url, &chain, NULL);
+        ok_(__FILE__,line)(res && GetLastError() == 0,
+                           "InternetGetSecurityInfoByURLA failed returned: %x(%u), expected success\n", res, GetLastError());
+
+        SetLastError(0);
+        res = pInternetGetSecurityInfoByURLA(url, NULL, &flags);
+        ok_(__FILE__,line)(res && GetLastError() == 0,
+                           "InternetGetSecurityInfoByURLA failed returned: %x(%u), expected success\n", res, GetLastError());
     }else {

Why not set last error as 0xdeadbeef in these two tests?

Actually (with a few exceptions when apps really depend on it), it's better to not test last error on success. Caller should never use it anyway and it might be polluted by internal calls that we're not interested in.

Wei, I sent a modified version of your patch. I hope you're fine with those changes. Thanks for contribution.

Jacek