Wrong comparison in ntdll/actctx.c

[nlataill]

Hi,

Working on a D3D game, I have found that wine is refusing to load the
following
manifest file (when Windows does not fail).

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<!-- Copyright (c) 1981-2001 Microsoft Corporation -->
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
    <noInheritable/>
    <assemblyIdentity type="win32" name="Microsoft.VC80.OpenMP" version="
8.0.50727.42" processorArchitecture="x86"
publicKeyToken="1fc8b3b9a1e18e3b"/>
    <file name="vcomp.dll" hash="0d009725bd5653af143fdfd9d8abf8adad533d16"
hashalg="SHA1"/>
</assembly>

Adding verbosity to actctx.c, it seems that this game is requesting version
8.0.50608.0, which does match major.minor,
but not the whole version number (*major.minor.build.revision)*.

trace:actctx:parse_assembly_identity_elem name=L"Microsoft.VC80.OpenMP"
version=8.0.50727.42 arch=L"x86"
fixme:actctx:parse_assembly_elem VERBOSE: wrong version FOUND: 8/0/50727/42
fixme:actctx:parse_assembly_elem VERBOSE: wrong version EXP: 8/0/50608/0
fixme:actctx:parse_manifest_buffer failed to parse manifest L"C:\Program
Files\Microsoft.VC80.OpenMP.manifest"
fixme:actctx:parse_depend_manifests Could not find dependent assembly L"
Microsoft.VC80.OpenMP"

According to MSDN, it seems only major.minor is considered for version
matching:
- http://msdn2.microsoft.com/en-us/library/aa374234.aspx : A version number
that changes only in the *build* or *revision* parts indicates that the
assembly is backward compatible with prior versions.
- http://msdn2.microsoft.com/en-us/library/aa375365.aspx: [...] are
manifests used to redirect the version of a side-by-side assembly to another
compatible version. The version that the assembly is being redirected to
should have the same major.minor values as the original version.

I am proposing the following one-liner to ntdll/actctx.c:

isa@isa:~/downloads/tmp$ diff -u wine-0.9.49/dlls/ntdll/actctx.c
/home/isa/downloads/wine-0.9.49/dlls/ntdll/actctx.c
--- wine-0.9.49/dlls/ntdll/actctx.c     2007-11-09 17:56:12.000000000 +0100
+++ /home/isa/downloads/wine-0.9.49/dlls/ntdll/actctx.c 2007-11-19 21:07:
35.000000000 +0100
@@ -1390,8 +1390,8 @@
         if (expected_ai)
         {
             /* FIXME: more tests */
-            if (assembly->type == ASSEMBLY_MANIFEST &&
-                memcmp(&assembly->id.version, &expected_ai->version,
sizeof(assembly->id.version)))
+            if (assembly->type == ASSEMBLY_MANIFEST &&
+                ((assembly->id.version.major != expected_ai->version.major)
|| (assembly->id.version.minor != expected_ai->version.minor)) )
             {
                 FIXME("wrong version\n");
                 return FALSE;

With this patch, the manifest (and related DLL) is loading correctly.
This does not prevent my game die shortly after, in D3D code this time.
Still work to do ;-)

Thanks,

Norbert Lataille

[Stefan Dösinger]

Am Montag, 19. November 2007 21:25:25 schrieb nlataill:
> Working on a D3D game, I have found that wine is refusing to load the
> following
> manifest file (when Windows does not fail).
>
> I am proposing the following one-liner to ntdll/actctx.c:

You can send patches to [e-mail]. Please use git-format-patch 
to create the diff file, and mail it as a regular attachment. You have to be 
subscribed to the list, otherwise your patch will end up in moderation, but 
you can turn off mail delivery so your inbox doesn't get spammed.
> According to MSDN, it seems only major.minor is considered for version
> matching:

The msdn is sometimes wrong, it is a good idea to include a test in the 
patch(see dlls/ntdll/tests/). This shows your change is correct, and prevents 
others from changing it back because it happens to fix a different bug.

[Robert Shearman]

nlataill wrote:
> Adding verbosity to actctx.c, it seems that this game is requesting 
> version 8.0.50608.0, which does match major.minor,
> but not the whole version number (/major.minor.build.revision)/.

...
> According to MSDN, it seems only major.minor is considered for version 
> matching:
> - http://msdn2.microsoft.com/en-us/library/aa374234.aspx : A version
>
> number that changes only in the /build/ or /revision/ parts indicates 
> that the assembly is backward compatible with prior versions.
> - http://msdn2.microsoft.com/en-us/library/aa375365.aspx: [...] are
>
> manifests used to redirect the version of a side-by-side assembly to 
> another compatible version. The version that the assembly is being 
> redirected to should have the same major.minor values as the original 
> version.
>
> I am proposing the following one-liner to ntdll/actctx.c:
>
> isa@isa:~/downloads/tmp$ diff -u wine-0.9.49/dlls/ntdll/actctx.c 
> /home/isa/downloads/wine-0.9.49/dlls/ntdll/actctx.c
> --- wine-0.9.49/dlls/ntdll/actctx.c     2007-11-09 17:56: 12.000000000 
> +0100
> +++ /home/isa/downloads/wine-0.9.49/dlls/ntdll/actctx.c 2007-11-19 
> 21:07:35.000000000 +0100
> @@ -1390,8 +1390,8 @@
>          if (expected_ai)
>          {
>              /* FIXME: more tests */
> -
>        if (assembly->type == ASSEMBLY_MANIFEST &&
> -
>            memcmp(&assembly->id.version, &expected_ai->version, 
>
> sizeof(assembly->id.version)))
> -
>        if (assembly->type == ASSEMBLY_MANIFEST &&
> -
>            ((assembly->id.version.major != 
>
> expected_ai->version.major) || (assembly->id.version.minor != 
> expected_ai->version.minor)) )
>              {
>                  FIXME("wrong version\n");
>                  return FALSE;
>
> With this patch, the manifest (and related DLL) is loading correctly.
> This does not prevent my game die shortly after, in D3D code this 
> time. Still work to do ;-)

I think the attached patch is more complete and implements what MSDN states.

[nlataill]

Hi,

Yes, I can see that such 4-level comparisons are already
in the actctx.c codebase (probably for a reason ?). Looking at
MSDN earlier this evening, I could not find reference to 3rd and
4th entries in comparisons, but I probably skipped the right section
in the doc.

Do you believe it is worthwhile to check this comparison function
from a windows test perspective ?

Thanks,

Norbert

2007/11/19, Robert Shearman :
> nlataill wrote:
> > Adding verbosity to actctx.c, it seems that this game is requesting
> > version 8.0.50608.0, which does match major.minor,
> > but not the whole version number (/major.minor.build.revision)/.
>
> ...
> > According to MSDN, it seems only major.minor is considered for version
> > matching:
> > - http://msdn2.microsoft.com/en-us/library/aa374234.aspx : A version
> >
> > number that changes only in the /build/ or /revision/ parts indicates
> > that the assembly is backward compatible with prior versions.
> > - http://msdn2.microsoft.com/en-us/library/aa375365.aspx: [...] are
> >
> > manifests used to redirect the version of a side-by-side assembly to
> > another compatible version. The version that the assembly is being
> > redirected to should have the same major.minor values as the original
> > version.
> >
> > I am proposing the following one-liner to ntdll/actctx.c:
> >
> > isa@isa:~/downloads/tmp$ diff -u wine-0.9.49/dlls/ntdll/actctx.c
> > /home/isa/downloads/wine-0.9.49/dlls/ntdll/actctx.c
> > --- wine-0.9.49/dlls/ntdll/actctx.c     2007-11-09 17:56: 12.000000000
> > +0100
> > +++ /home/isa/downloads/wine-0.9.49/dlls/ntdll/actctx.c 2007-11-19
> > 21:07:35.000000000 +0100
> > @@ -1390,8 +1390,8 @@
> >          if (expected_ai)
> >          {
> >              /* FIXME: more tests */
> > -
> >        if (assembly->type == ASSEMBLY_MANIFEST &&
> > -
> >            memcmp(&assembly->id.version, &expected_ai->version,
> >
> > sizeof(assembly->id.version)))
> > -
> >        if (assembly->type == ASSEMBLY_MANIFEST &&
> > -
> >            ((assembly->id.version.major !=
> >
> > expected_ai->version.major) || (assembly->id.version.minor !=
> > expected_ai->version.minor)) )
> >              {
> >                  FIXME("wrong version\n");
> >                  return FALSE;
> >
> > With this patch, the manifest (and related DLL) is loading correctly.
> > This does not prevent my game die shortly after, in D3D code this
> > time. Still work to do ;-)
>
> I think the attached patch is more complete and implements what MSDN
> states.
>
> --
> Rob Shearman