Skip to content

Conversation

@kpreisser
Copy link
Contributor

@kpreisser kpreisser commented Sep 4, 2019

Contributes to #2796

Summary:

  • Fixed the CopyMemory entrypoint declaration, which is not working on .NET Core 3.0 as kernel32.dll doesn't have an CopyMemory export.

Changes:

  • Changed EntryPoint from CopyMemory to RtlCopyMemory
  • Changed the numberOfBytes parameter from uint to UIntPtr

Note: I believe using an pointer type (IntPtr or UIntPtr) for the numberOfBytes parameter is correct even though pinvoke.net (and other sites) specify it as int, because the documentation for CopyMemory/RtlCopyMemory declares it as SIZE_T which is defined as pointer-sized integer (ULONG_PTR). Additionally, I have done some testing to verify that the value is actually interpreted as 64-bit integer when running as x64 (by specifying a value greater than 0xFFFFFFFF and verifying the correct number of bytes was copied).

Note: There is also a managed API Buffer.MemoryCopy() that could be used instead of p/invoking RtlCopyMemory/RtlMoveMemory, available starting with .NET Framework 4.6, but that one seems to be slower.

How Has This Been Tested?

Tested manually using CefSharp.MinimalExample.NetCore.Wpf (see cefsharp/CefSharp.MinimalExample#57)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • The formatting is consistent with the project (project supports .editorconfig)

@AppVeyorBot
Copy link

@amaitland
Copy link
Member

  • Changed EntryPoint from CopyMemory to RtlMoveMemory

Is there a reason you chose RtlMoveMemory over RtlCopyMemory? I see no reason to use the slower move memory.

Note: There is also a managed API Buffer.MemoryCopy() that could be used instead of p/invoking RtlMoveMemory, available starting with .NET Framework 4.6, but that one seems to be slower.

As we only target .Net 4.5.2 this isn't an option anyway.

@amaitland amaitland changed the title Fix the entrypoint declaration for CopyMemory WPF - Modify CopyMemory EntryPoint to work with .Net Core Sep 5, 2019
@amaitland amaitland added the wpf WPF Implementation label Sep 5, 2019
@AppVeyorBot
Copy link

Build CefSharp 76.1.90-CI3259 completed (commit 9233614c43 by @)

@kpreisser
Copy link
Contributor Author

Is there a reason you chose RtlMoveMemory over RtlCopyMemory? I see no reason to use the slower move memory.

No particular reason, I was just following the comments in https://github.com/dotnet/coreclr/issues/24008 for changing CopyMemory to RtlMoveMemory (I assume because it doesn't require buffers to not overlap and so is safer).

I updated the PR to use the faster RtlCopyMemory instead.
Thanks!

@AppVeyorBot
Copy link

@amaitland amaitland added this to the 77.0.0 milestone Sep 5, 2019
@amaitland amaitland merged commit c311a58 into cefsharp:master Sep 6, 2019
@amaitland
Copy link
Member

amaitland commented Sep 6, 2019

(I assume because it doesn't require buffers to not overlap and so is safer).

Actually it would appear to be that RtlCopyMemory only exists for x64 as detailed in https://stackoverflow.com/a/47016390/4583726

Confirmed in the output of dumpbin /exports kernel32.dll there is indeed no RtlCopyMemory.

#define RtlCopyMemory(Destination,Source,Length) memcpy((Destination),(Source),(Length))

winnt.h calls memcpy, so changing to directly call memcpy appears to be one option.

The other being to use NativeMethodWrapper.CopyMemoryUsingHandle which I had previous added with the intention of bench marking to see if the native call was quicker than the p/invoke call. (Crappy method name I know, from memory it I couldn't call it CopyMemory or RtlCopyMemory as the macros interfere with the method name).

Additional background http://www.vbforums.com/showthread.php?856067-Which-Windows-DLL-actually-has-RtlCopyMemory&p=5238099&viewfull=1#post5238099

amaitland added a commit that referenced this pull request Sep 6, 2019
RtlCopyMemory was only working for 64bit, falling back to RtlMoveMemory for now.

Details in #2885 (comment)

Follow up to c11157f
amaitland added a commit that referenced this pull request Sep 6, 2019
- RtlCopyMemory only works on x64, we could use the slower RtlMoveMemory, going with a native wrapper around RtlCopyMemory
- Renamed CopyMemoryUsingHandle to MemoryCopy (cannot be CopyMemory or RtlCopyMemory as macro with that name already exists)

#2885 (comment)

Issue #2885
@amaitland
Copy link
Member

Changed to use native wrapper around RtlCopyMemory in b1b9306

Originally the Rendering implementation in WPF was written in VC++ and used MemoryCopy so I'm comfortable it's well tested.

@kpreisser
Copy link
Contributor Author

kpreisser commented Sep 7, 2019

Thank for your the background information and the fix! Sorry, I didn't test running as x86 after changing the PR from RtlMoveMemory to RtlCopyMemory, so I didn't notice it was missing on x86.

(As additional thought, since kernel32's RtlMoveMemory gets forwarded to NTDLL.RtlMoveMemory, it might be safer to use that one instead of kernel32.RtlMoveMemory as it is properly documented, whereas kernel32.RtlMoveMemory has only archived documentation that wasn't clear about the symbol name. But no need to change that now of course since using memcpy from the CRT should be fine and is slightly faster.)

Thanks!

@kpreisser kpreisser deleted the fixCopyMemory branch September 7, 2019 08:53
@amaitland
Copy link
Member

Sorry, I didn't test running as x86 after changing

All good 😄 Makes no sense that it exists on x64 and not x86.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wpf WPF Implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants