8

Am I doing this right?

I get a pointer to a native array and need to copy to a managed array. Use memcpy() with a pin_ptr.

unsigned char* pArray;
unsigned int arrayCount;
// get pArray & arrayCount (from a COM method) 

ManagedClass->ByteArray = gcnew array<Byte,1>(arrayCount)
pin_ptr<System::Byte> pinPtrArray = &ManagedClass->ByteArray[0];
memcpy_s(pinPtrArray, arrayCount, pArray, arrayCount);

arrayCount is the actual length of pArray, so not really worried about that aspect. Looked at the code and the array is copied from a vector. So I can set the managed array size safely.

Luke Narramore
  • 502
  • 2
  • 6
  • 17

2 Answers2

12

That works, but isn't safe. You'll blow the garbage collected heap to smithereens when you get arrayCount wrong. Very hard to diagnose.

Marshal::Copy() is safe and just as fast.

Hans Passant
  • 922,412
  • 146
  • 1,693
  • 2,536
  • 2
    Marshal::Copy is NOT safe. Here's one example: http://stackoverflow.com/questions/6945998/copy-from-const-char-to-a-byte-array-c-c-interop-marshalcopy/6946226#6946226 – Ed Bayiates Aug 04 '11 at 18:17
  • 2
    Getting an immediate AV from fumbling the native buffer size is not comparable to the misery caused by silent heap corruption. – Hans Passant Aug 04 '11 at 18:32
  • 1
    Hans: you do not always get an immediate AV. Marshal::Copy can corrupt the heap the same way as memcpy does. The calls are similar. – Ed Bayiates Aug 04 '11 at 18:35
  • 1
    No, Marshal::Copy() *always* checks if the array indexing is out of bounds. Memcpy() *never* does. It is important to distinguish an AV caused by *writing* the managed data vs *reading* the unmanaged data. – Hans Passant Aug 04 '11 at 18:38
  • 1
    It does with some overloads. But not all. – Ed Bayiates Aug 04 '11 at 18:39
  • 1
    No, *all* of the overloads check. There are only two actual implementations for these overloads, CopyToManaged() and CopyToNative(). They are marked as internal calls, their code is written in C++ in the CLR. Which uses memcpy() after the checks. You can find this back in the SSCLI source code. – Hans Passant Aug 04 '11 at 19:17
  • 2
    No, some do NOT check. Read the documentation: Unmanaged, C-style arrays do not contain bounds information, which prevents the startIndex and length parameters from being validated. Thus, the unmanaged data corresponding to the source parameter populates the managed array regardless of its usefulness. You must initialize the managed array with the appropriate size before calling this method. – Ed Bayiates Aug 04 '11 at 19:18
  • 1
    What you are missing out on is the protection that Marshal::Copy() provides against corrupting the *garbage collected heap*. It doesn't store a C array, it is a managed array that can *always* be bounds-checked. Getting the Marshal::Copy() call wrong either generates an immediate managed exception, junk data in the managed array or an immediate AV. Never silent heap corruption. Copying *from* a managed array *to* a C array can cause the usual havoc but that's not what the OP is trying to do. – Hans Passant Aug 04 '11 at 19:29
  • Hans, it doesn't bounds check. It does a simple length check. I've edited my answer to include it. – Ed Bayiates Aug 04 '11 at 19:38
  • Yes, bounds checking is simple. – Hans Passant Aug 04 '11 at 19:43
  • 1
    Yes, which is why you should use pinning, and not Marshal::Copy from C++/CLI. – Ed Bayiates Aug 04 '11 at 19:45
  • 3
    No, you assume that pinning is for free. It is not. The CLR can do it without pinning. clr/src/vm/common.h, memcpyNoGCRefs() function. – Hans Passant Aug 04 '11 at 19:50
3

You are doing it almost right:

pin_ptr<Byte> pinPtrArray = &ManagedClass.ByteArray[ManagedClass.ByeArray->GetLowerBound(0)];

Marshal::Copy is not safe and not as fast. Always use pinned pointers in managed C++.

Edit: If you want to, you can check the length to make sure the memcpy won't exceed the bounds first, e.g.:

if (arrayCount > ManagedClass.ByteArray.Length)
    (throw Out of bounds copy exception)
Ed Bayiates
  • 11,060
  • 4
  • 43
  • 62
  • arrayCount is the actual length of pArray, so not really worried about that aspect. Looked at the code and the array is copied from a vector. So I can set the managed array size safely. – Luke Narramore Aug 05 '11 at 09:02
  • Good. I was just responding to the unreasonable doom-speak of Hans below, acting as if things like memcpy were evil. Sigh. – Ed Bayiates Aug 05 '11 at 15:15
  • @AresAvatar : `memcpy` isn't evil, but Hans is right -- `Marshal::Copy` **is** safer (and is **not** slower). – ildjarn Aug 10 '11 at 20:31
  • @ildjarn, Marshall::Copy is absolutely slower. I don't understand how anyone could deny it. memcpy maps to a very few processor instructions and in fact with optimizations the compiler does not even use a function call for it. Study the disassembly. How can you compare a function call with branching inside it to a small set of intrinsic memory move instructions? – Ed Bayiates Aug 10 '11 at 20:41
  • @AresAvatar : `Marshal::Copy` is implemented in terms of `memcpy`. Likely, you're looking at the disassembly with JIT optimizations disabled, because the disassembly is in fact identical once the bounds checking is performed. – ildjarn Aug 10 '11 at 21:05
  • @ildjarn, if what you are saying is true, then my code is identical to Marshal::Copy with the edit. So what's the point of your comment? – Ed Bayiates Aug 10 '11 at 21:07
  • 1
    @AresAvatar : The point is your calling Hans' answer "doom-speak" is unwarranted and incorrect. – ildjarn Aug 10 '11 at 21:08
  • @ildjarn, it certainly is neither unwarranted nor incorrect. My whole point is that memcpy is perfectly fine, and you are just confirming that. Hence Hans is way off base. You've just helped me prove that Marshal::Copy is not safer than my code above, and not faster either (as it involves a function call). You seem to be speaking with some kind of personal loyalty to Hans rather than logic or scientific evidence. – Ed Bayiates Aug 10 '11 at 21:13
  • 1
    @AresAvatar : `memcpy` is just as safe **IF** you do the bounds checking **AND** you do it correctly. `Marshal::Copy` is thus safer (because it does these things for you) and shorter (because it does these things for you). In any case, I'm done trying to persuade you that any answer other than yours warrants any merit, or that your FUD is more harmful than not. – ildjarn Aug 10 '11 at 21:18
  • @ildjarn - it's not FUD. It's FACT. – Ed Bayiates Aug 10 '11 at 21:22
  • @AresAvatar : Your "fact" that `memcpy` is faster is wrong, and your "fact" that `Marshal::Copy` is not safer is wrong -- maybe you need a remedial debugging course _and_ a remedial English course... ;-] – ildjarn Aug 10 '11 at 21:27
  • @ildjarn, please stop these childish insults. You are wrong, and you know (or should know) you are wrong. This is not a good use of space on StackOverflow. – Ed Bayiates Aug 10 '11 at 21:30