System.AccessViolationException on Windows 10 (64 Bit) after 64 bit-fix?

[anothertal3]

Hello Bassman2,

after my successful performance experiments in #7 using a Windows 7 x64 PC I wanted to repeat the same for my main machine running Windows 10 (v1803) x64.

However, my WPF application always crashes during the run with the following exception.

An unhandled exception of type 'System.AccessViolationException' occurred in mscorlib.dll
Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

Sadly I haven't been able to catch this exception or any corresponding callstack at all and I don't really know, which operations actually fails.

The test does the following:

1. Enumerate all files using root.EnumerateFileSystemInfos("*", SearchOption.AllDirectories)
2. Store MediaFileInfo handle for each file in a list
3. Iterate over that list and copy everything to a folder on my PC using MediaFileInfo.CopyTo

The exception always triggers during this run but sometimes during phase 2 and during phase 3.

One thing I was able to determine: the issue only appears after commit 436ab2e which included the "64-bit fix" for #6. Using the commit before ( 01a6e6a) works fine (albeit slow).

To be honest: I'm uncertain about the code changes you implemented then and what they actually affect. Do you have any suggestions on how to analyze this appropriately?

Best regards,
tale

[Bassman2]

I'm workin on it

[anothertal3]

Thanks a lot for your hard work!

[molzahn-sio]

@Bassman2 Sorry to bother, but has there been any progress in that regard?
I could offer to run some code examples on my PC if that proves helpful?

[Bassman2]

Hi ralmo, feel free to help me. It's not so easy to catch the problem because it is sporadic and the exception is catched by the system.

[molzahn-sio]

I'm glad you were at least able to confirm the issue at all.

Not sure if I'm of much help with the actual technical implementation but maybe I can manage to get additional information about how to reproduce the issue more reliable or find one of the originating code line.

[Bassman2]

The problem is that the exception is catched by the system but I need a full stack trace for analysis. That should be possible with this attributes for the calling function.

[HandleProcessCorruptedStateExceptions]
[SecurityCritical]

But I cannot reproduce it with this attribues.
If somone has more luck please send me the full exception stack trace.

[Darkyenus]

I have also encountered this problem, but it appears too rarely to be easily reproduced (I have to do complex operations in my application while repeatedly plugging-in/out the device to even have a chance at reproducing it). And then it crashes without any stack trace, like reported above. I'll add suggested attributes and report back if I find any more info.

However, what is more worrying, is that it happens even when the application is running in 32bit mode, which worked well before. Therefore I propose reverting all changes which attempt to make 64bit mode work (436ab2e, maybe more?) and making a new, 32bit only release. It would have all the improvements which were added since that commit, but would be stable for those who rely on the library, and don't need it to be 64bit. Further attempts to make 64bit work could be done on an experimental branch, which would be merged once we figure out and fix what is going wrong now.

Thoughts?

[anothertal3]

@Bassman2 As always, thanks for your hard work. I will do some tests during the weekend with those attributes and report back.

@Darkyenus Your suggestion would be a sensible approach until a fix is determined. The performance optimizations are a huge advancement which I'd love to make use of but cannot right now.

[anothertal3]

I tried adding these attributes to almost every method in my call hierarchy but it didn't change anything.

I then tried some semi-desperate measures and added a huge load of debug prints. The line where it finally breaks is within PropVariant.cs:

        public static PropVariant FromValue(PROPVARIANT value)
        {
            System.Diagnostics.Debug.WriteLine($"FromValue: value = {value}");

            var size = Marshal.SizeOf(value);
            System.Diagnostics.Debug.WriteLine($"FromValue: sizeof value = { size}");

            IntPtr ptrValue = Marshal.AllocHGlobal(size);
            System.Diagnostics.Debug.WriteLine($"FromValue: new pointer = {ptrValue} will now structureToPtr");

            Marshal.StructureToPtr(value, ptrValue, false);
            System.Diagnostics.Debug.WriteLine($"FromValue: structureToPtr was fine, marshalling back to c#");

            //
            // Marshal the pointer into our C# object
            //

            var result = (PropVariant)Marshal.PtrToStructure(ptrValue, typeof(PropVariant));
            System.Diagnostics.Debug.WriteLine($"FromValue: conversion successful = {result}");

            return result;
        }

Here's the "call stack" after EnumerateFileSystemInfos -> GetChildren:

GetChildren: try create \Interner gemeinsamer Speicher\DCIM\Camera
Item: start with o1FD, \Interner gemeinsamer Speicher\DCIM\Camera
Item: will refresh
Refresh: start
Refresh: not root, get properties
GetProperties: start
GetProperties: got values
GetProperties: get value at 0
GetProperties: got them -> key=PortableDeviceApiLib._tagpropertykey, val=PortableDeviceApiLib.tag_inner_PROPVARIANT
GetProperties: get value at 1
GetProperties: got them -> key=PortableDeviceApiLib._tagpropertykey, val=PortableDeviceApiLib.tag_inner_PROPVARIANT
GetProperties: ContentType, key=PortableDeviceApiLib._tagpropertykey, val=PortableDeviceApiLib.tag_inner_PROPVARIANT
FromValue: value = PortableDeviceApiLib.tag_inner_PROPVARIANT
FromValue: sizeof value = 24
FromValue: new pointer = 235630056 will now structureToPtr
FromValue: structureToPtr was fine, marshalling back to c#
FromValue: conversion successful = ef2107d5-a52a-4243-a26b-62d4176d7603
GetProperties: get value at 2
GetProperties: got them -> key=PortableDeviceApiLib._tagpropertykey, val=PortableDeviceApiLib.tag_inner_PROPVARIANT
GetProperties: Name
FromValue: value = PortableDeviceApiLib.tag_inner_PROPVARIANT
FromValue: sizeof value = 24
FromValue: new pointer = 235630216 will now structureToPtr
FromValue: structureToPtr was fine, marshalling back to c#
FromValue: conversion successful = IMG_20180926_230413.jpg
GetProperties: get value at 3
GetProperties: got them -> key=PortableDeviceApiLib._tagpropertykey, val=PortableDeviceApiLib.tag_inner_PROPVARIANT
GetProperties: OriginalFileName
FromValue: value = PortableDeviceApiLib.tag_inner_PROPVARIANT
FromValue: sizeof value = 24
FromValue: new pointer = 235630568 will now structureToPtr
FromValue: structureToPtr was fine, marshalling back to c#
An unhandled exception of type 'System.AccessViolationException' occurred in mscorlib.dll
Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

I hope this helps somehow?

[Darkyenus]

I have been digging around the documentation, trying to verify if the reported size of PROPVARIANT is correct, but my findings were inconclusive - I am not sure how is this all handled on 64bit systems. But what I did found is the documentation of PROPVARIANT where they claim that it is the same size as DECIMAL (16 bytes, I think).

Also, I am not sure if this is not just a problem with my code, but I suspect that there is a native memory leak somewhere. When I do something like

var folder = device.GetFileInfo(path);
while (true)
    folder.Refresh();

the native memory keeps steadily increasing, up to an out of memory error. I am not opening a separate issue for this yet, because I haven't yet created a minimal repro case to confirm that it really is a problem with the library, but I am mentioning it here, because it may be related.

[Darkyenus]

I have been doing some experiments and have come to a conclusion, that having 64bit support alongside 32bit, in the same dll, is unfeasibly hard.

I'll document my findings here, as it may be useful to others and to my future self. Some/all of this may be wrong, so if you spot any mistakes, please let me know.

Struct layouts

Checks with native C++ have confirmed, that PROPVARIANT is indeed 16 bytes wide on 32bit and 24bit wide on 64bit. However, it is impossible to have C# struct with different layout on 32/64bit. So we would have to have two different dlls, one with 32bit struct layouts and one for 64bit. Those would have to be distributed separately, as Windows don't support "multi-architecture binaries".

64bit COM wrapper dll

The library depends on slightly modified Interop.PortableDevicesApi.dll (etc.), which is generated by tlbimp from C:/Windows/System32/PortableDevicesApi.dll (64bit version) (or C:/Windows/SysWOW64/PortableDevicesApi.dll (32 bit version, windows is weird)). ("our" .dll contains automatically generated .NET bindings for the COM library in C:/Windows.) It is possible to specify architecture for which the bindings should be generated (x86, x64 or agnostic) and I have tried them all. The dll generated for x86 is identical to the one generated for "Agnostic", except for 32BITREQUIRED flag in .corflags. But the struct layouts are identical in both.

Generating x64 wrapper is trickier, as tlbimp gets redirected to the 32bit dll and complains that it is not compatible with x64 (at least on my machine: TlbImp : error TI2010 : A single valid machine type compatible with the input type library must be specified.), so I had to copy it out of System32 and then do the conversion. The resulting .dll was mostly the same, except for layouts of most structs, which had bigger packing, and few types were different (Int32 -> Int64).

I have also tried to generate .dlls through midl, as described here, to see if it would fix the broken layout of unions (such as PROPVARIANT), but it didn't help. Generated .dlls were identical, just some members were shuffled around and for some reason there was a new binding to PortableDeviceApi.IPortableDeviceWebControl, but we don't need that, as far as I know.

Memory leaks

It seems that the memory leak I mentioned in the previous comment and in #15 is unrelated to 64-bitness, but is still troubling. Type marshalling done by the wrapper should have been enough to ensure correct memory management, but it seems that this is possible only for reference types (i.e. objects, not structs), in particular for strings and COM objects, implementing IUnknown.

However PROPVARIANT is just a struct, which may, or may not hold some other resources (e.g. pointer to string), so it is probably not so easy to do its cleanup correctly and the marshaller either doesn't do it at all, or it does not do it reliably (which would be probably harder to deal with). I am still not entirely sure how this works (I am looking for learning resources), but I have managed to do the cleanup manually (see #15) and it does fix the problem. But it isn't localized to one place, and many parts of the API will have to be redesigned to accomodate the need to do the resource freeing (for example methods which return IEnumerable<PropVariant> are particularly problematic). Furthermore, I am not sure if there aren't any more cases of structs which need this treatment.

[anothertal3]

@Darkyenus : Sadly, this appears to be a little out of my league so I can't really suggest an appropriate solution. But if I understand you correctly, MediaDevices will most likely need to be deployed with different dll's for 32/64 bit systems?

[Darkyenus]

@anothertal3 Yes, I think so. But maybe it would be possible to do it so that the runtime automatically loads the correct dll for the platform, but I don't know how hard that is in C# land.

[anothertal3]

Haven't tried something like that but there seem to be options, e.g. https://stackoverflow.com/a/2594135

So, regarding the actual files that need to be used: Switching between a Interop.PortableDeviceApiLib.dll for 32 bit (from your fork?) and 64 bit (from the latest release) should do the trick?

[Darkyenus]

@anothertal3 Yes, loading different DLL could work well. I am currently shipping with 32bit (default) build of my cleanup branch and I have not had any problems with it (except for those which were caused by Android itself and their slightly idiotic need to refresh manually after each filesystem change, which doesn't properly support file removal, but I digress).

Changes made in that fork:

• Completely regenerate and rebuild PortableDeviceApi/Types COM wrapper DLLs, with detailed documentation of every change done. I have used newer tlbimp than what was used originally, which generated a slightly different/more detailed wrapper. This was mostly beneficial, but some parts needed further slight modifications (full description of the wrapper modifications).
• Fixed a memory leak of PROPVARIANT. Every time PROPVARIANT was retrieved from COM, it was not freed and would leak. This is one of the most used structs of the API, so it caused my app to run out of memory in about 2 hours. The library originally used PropVariant wrapper struct, but with improved and slighly modified COM wrapper, this was no longer strictly necessary, so I removed the struct and started to use the COM wrapper version directly, with some convenience extension functions. This had an additional benefit in regard to 32/64bit portability - the original PropVariant struct needed to have explicit layout, which could only be correct for 32bit or 64bit, but not both. By using the struct from COM wrapper DLLs, the layout is always correct.
• I have done the COM wrapper regeneration twice (along with all necessary changes). Once for 32bit and once for 64bit. So now it should be possible to simply change the dependencies of the library from x86 folder to the same DLLs in x64 folder and have a 64bit version, which could be dynamically loaded instead of the 32bit version. I say should instead of is, because I haven't tested that, so I am not sure if it would really work. I don't currently need 64bit version of this library, so I don't plan on doing that, but I will outline what needs to be done if anybody needs it:
  • Go through the codebase and check if there are no more structs with explicit layout. If there are any, they will need to be either replaced with their counterparts from COM wrapper DLLs, or introduce some #ifdef building-for-64-bit style conditionals to ensure that the layout is correct. I am not aware of any such structs, but I am not 100% sure.
  • Change the dependencies on PortableDeviceApi/Types from x86 to x64 folder. Maybe it would be possible to introduce a second type of build with these settings, but I am not familiar enough with Visual Studio to know if that is possible. I think it is, but I don't know how.
  • Build it (if it even compiles) and test it in 64-bit application, if everything works as it should, without crashes, leaks, etc.
  • Somehow ensure that your application uses the correct MediaDevices.dll for the architecture. I.e. something like the stackoverflow link which @anothertal3 posted. Test if switching platforms really does work and that it correctly uses the appropriate struct layout for everything. I don't know how much does C# inline these things from its dependencies - this could be a problem if it does. (My guess would be that it doesn't, but as I said before, I don't know.)

That is basically it. I would suggest against mixing my forked version with the original, because there may have been some minor API changes which could cause problems down the line and also because of that fixed memory leak.

If @Bassman2 is insterested, I will create a PR for the changes on the cleanup branch. I admit that they are not very modular, but I think that they would benefit this project.

[Bassman2]

Hi All, I added platform handling to the MediaDevices.csproj to switch between different interops made for any, x86 and x64. I hope this will help.

[anothertal3]

@Bassman2 : I was able to build a "x64" variant and successfully execute my tests with it. A "x86" build crashes immediately on a 64 bit machine (as expected). I assume one would need to create to application setups for deployment then.

@Darkyenus : Thanks for your detailed input. I tested your forked build as well, but wasn't able to get it to work on my 64 bit machine (32 was fine). I've not yet committed a lot of time, though, so maybe there was a simple oversight on my end.

That is basically it. I would suggest against mixing my forked version with the original, because there may have been some minor API changes which could cause problems down the line and also because of that fixed memory leak.

The only change I made was changing the referenced Project from @Bassman2 's MediaDevices to the fork so I'm uncertain if anything more had to be done?

[Bassman2]

I can currently not reproduce it with the new Facade branch. The x64 build works for x64 apps, the x86 build works for x86 apps and the "Any CPU" build works for both with on exception. ToByteArray does not work with "Any CPU". Does anyone have an idea? Please try if you can confirm my result.

[anothertal3]

I can confirm that x64 seems to be working. I'm not around a different machine, however, to test the other right now.

[Bassman2]

To avoid the use of different dlls for x86 and x64 I tried to stop working with the generated COM wrapper classes but to implement the COM wrapper classes manually. This gives me more control over the COM wrapper classes. A first attempt is in the "manual" branch.

[Bassman2]

Fixed with own COM wrapper classes

[molzahn-sio]

Can confirm that this worked on x64.
(I've compiled it using "any cpu" for the mediadevices project.)

I did have a strange crash at first but all further attempts were successful.