Re: crypt32: Remove a test because of a Windows 2003 SP1 bug
Juan Lang wrote:
- /* Now check just the time */ - flags = CERT_STORE_TIME_VALIDITY_FLAG; - parent = CertGetIssuerCertificateFromStore(store, child, NULL, &flags); - ok(parent != NULL, "CertGetIssuerCertificateFromStore failed: %08x\n", - GetLastError()); - /* Oops: the child is not expired, so the time validity check actually - * succeeds, even though the signing cert is expired. + /* Checking time validity is not productive, because while most Windows + * versions return 0 (time valid) because the child is not expired, + * Windows 2003 SP1 returns that it is expired. Thus the range of + * possibilities is covered, and a test verifies nothing. */
The test being removed is correct and valid. Removing it could lead to a regression in Wine as this case is not being covered when this patch is applied. While a 100% pass rate is ideal, on Windows 2003 SP1 that test *is* failing due to a bug. However, this is the correct behaviour, and the bug is detected by this test. - Reece
The test being removed is correct and valid.
Are you certain? Remember that I wrote it ;)
While a 100% pass rate is ideal, on Windows 2003 SP1 that test *is* failing due to a bug.
Again, are you sure? Or are you just saying that because I said it is in the commit? The definition of "bug" is sort of fluid here. I wrote the test to check a fairly strange phenomenon: a cert that is time valid is asked for its issuer, which happens *not* to be time valid. The time valid flag is cleared in the process. It's not entirely clear which cert the flag is supposed to apply to, and the result surprised me when I wrote the test. I call it a bug because the behavior is one way on Windows 2003 SP1, but different on XP. It could be that it was in fact a bug in Windows XP, which was fixed in Windows 2003, but reverted because some app depended on the behavior. Furthermore, this function (CertGetIssuerFromStore) has been deprecated in favor of CertGetCertificateChain, which I have implemented and tested rather extensively. There appears to be no disagreement over which cert the time valid flag applies to with CertGetCertificateChain. So I'm not sure that I share your reservation about removing the test. --Juan
On 25/10/2007, Juan Lang <juan.lang(a)gmail.com> wrote:
The test being removed is correct and valid.
Are you certain? Remember that I wrote it ;)
:)
While a 100% pass rate is ideal, on Windows 2003 SP1 that test *is* failing due to a bug.
Again, are you sure? Or are you just saying that because I said it is in the commit?
I was basing it on what you said in the commit - both the heading and the comment in the patch.
The definition of "bug" is sort of fluid here. I wrote the test to check a fairly strange phenomenon: a cert that is time valid is asked for its issuer, which happens *not* to be time valid. The time valid flag is cleared in the process. It's not entirely clear which cert the flag is supposed to apply to, and the result surprised me when I wrote the test.
Going on what you say here would make XP have the bug, and Windows 2003 SP1 containing a fix for the behaviour.
I call it a bug because the behavior is one way on Windows 2003 SP1, but different on XP. It could be that it was in fact a bug in Windows XP, which was fixed in Windows 2003, but reverted because some app depended on the behavior.
I agree that the differing behaviour is due to a bug. It also makes sense that the bug is in XP, and the fix was reverted because of application compatibility.
Furthermore, this function (CertGetIssuerFromStore) has been deprecated in favor of CertGetCertificateChain, which I have implemented and tested rather extensively. There appears to be no disagreement over which cert the time valid flag applies to with CertGetCertificateChain.
That is good :). Based on what you are saying above, it looks as if MS created CertGetCertificationChain in part to address this bug, and are moving applications over to the corrected behaviour API, while retaining the old behaviour.
So I'm not sure that I share your reservation about removing the test.
I know nothing of the API you are implementing, apart from what you mentioned here and in the patch. You clearly have a solid understanding of what is involved here. That said, imagine the following scenario: 1. This patch is applied to remove the test for this feature^Wbug in CertGetCertificateChain; 2. Someone in the future submits a patch to fix the 'bug' in CertGetCertificateChain for Wine; 3. A user attempts to run an application that is dependent on CertGetCertificateChain an d its broken behaviour, which fails to work and they start complaining. Based on what you said above, it looks as if something like this happened with Windows 2003. Thus, if the results from this test highlight a bug in Windows, surely the test should remain so that Wine can be bug-for-bug compatible. Even if this is a deprecated API, there will still be applications that are dependent on it. That is my rationale for keeping the test. Keep up the great work, - Reece
I was basing it on what you said in the commit - both the heading and the comment in the patch.
That's dangerous here. The details are small, but they matter:
1. This patch is applied to remove the test for this feature^Wbug in CertGetCertificateChain;
No, the feature/bug is in CertGetIssuerCertificateFromStore. And the test tested for a single bit being cleared. In Windows 2003 SP1, that bit is instead set. So leaving the test as-is is misleading: it fails on at least one Windows platform (Windows 98 is unknown, because the crypt32 tests aren't running there for some reason that I don't know.) Changing it to cover both possibilities is my usual tactic, but in the case of a single bit, there are only two possibilities. Hence the comment in the patch: the bit being either set or cleared is acceptable on some system on which the test is run. How, then, is the test meaningful?
2. Someone in the future submits a patch to fix the 'bug' in CertGetCertificateChain for Wine;
No - wrong function, and one which is not inconsistent in at least this detail on different versions of Windows.
Even if this is a deprecated API, there will still be applications that are dependent on it.
Of course, and the tests for the behavior of this API that is consistent remain. I just remove the test of this single bit, and document why. --Juan
On 25/10/2007, Juan Lang <juan.lang(a)gmail.com> wrote:
I was basing it on what you said in the commit - both the heading and the comment in the patch.
That's dangerous here. The details are small, but they matter:
Of course the details matter.
1. This patch is applied to remove the test for this feature^Wbug in CertGetCertificateChain;
No, the feature/bug is in CertGetIssuerCertificateFromStore.
Yeah, I copied the wrong function name. My mistake.
And the test tested for a single bit being cleared. In Windows 2003 SP1, that bit is instead set. So leaving the test as-is is misleading: it fails on at least one Windows platform (Windows 98 is unknown, because the crypt32 tests aren't running there for some reason that I don't know.) Changing it to cover both possibilities is my usual tactic, but in the case of a single bit, there are only two possibilities. Hence the comment in the patch: the bit being either set or cleared is acceptable on some system on which the test is run. How, then, is the test meaningful?
Testing both cases would be silly in this instance, as you say in the comment, as it would be like testing true or false is returned from a function. The only other thing would be for either Windows to agree (which it doesn't), or there be some documentation to describe the correct behaviour, or otherwise document the bug in one of the Windows versions. Assuming that you haven't found any, there is no reference to deduce what the result should be. Therefore, I withdraw my initial objection.
2. Someone in the future submits a patch to fix the 'bug' in CertGetCertificateChain for Wine;
No - wrong function, and one which is not inconsistent in at least this detail on different versions of Windows.
I meant to reference the other function. My mistake.
Even if this is a deprecated API, there will still be applications that are dependent on it.
Of course, and the tests for the behavior of this API that is consistent remain. I just remove the test of this single bit, and document why.
I retract my initial comments: the patch makes sense now. - Reece
participants (2)
-
Juan Lang -
Reece Dunn