Devin Jenson's WebLog

That One Guy

The Danger of Marshal.StructureToPtr

Pop Quiz! Is the following code secure:


public static unsafe IntPtr WriteListToNewNativeBuffer<T>(
    List<T> list)
{
    if (list == null)
    {
        throw new ArgumentNullException("list");
    }

    IntPtr nativeBuffer = IntPtr.Zero;
    Int32 elementSize = Marshal.SizeOf(typeof(T));

    try
    {
        nativeBuffer = Marshal.AllocCoTaskMem(list.Count * elementSize);
        Byte* bytePtr = (Byte*)nativeBuffer.ToPointer();

        foreach (T element in list)
        {
            Marshal.StructureToPtr(element, new IntPtr(bytePtr), false);
            bytePtr += elementSize;
        }

        return nativeBuffer;
    }

    catch
    {
        if (nativeBuffer != IntPtr.Zero)
        {
            Marshal.FreeCoTaskMem(nativeBuffer);
        }

        throw;
    }
}

Assuming the caller is properly freeing up memory, and assuming I didn't mess up the blog translation, it sure looks like all bases are covered. As you expected me to conclude, this code is insecure -- it can lead to a buffer overflow. Take this program which demonstrates the problem:


using System;
using System.Collections.Generic;
using System.Runtime.InteropServices;



class Program
{
    [StructLayout(LayoutKind.Sequential)]
    public class Base
    {
        public Int32 One;

        public Base(Int32 one)
        {
            One = one;
        }
    }

    [StructLayout(LayoutKind.Sequential)]
    public class Derived : Base
    {
        public Int32 Two;

        public Derived(Int32 one, Int32 two) : base(one)
        {
            Two = two;
        }
    }

    private static void Main()
    {
        List<Base> list = new List<Base>();
        list.Add(new Base(1));
        list.Add(new Derived(2, 3));

        IntPtr nativeBuffer = WriteListToNewNativeBuffer<Base>(list);
        Marshal.FreeCoTaskMem(nativeBuffer);
    }
}

The root of the problem is that Marshal.StructureToPtr only takes a basic System.Object parameter. Our WriteListToNewNativeBuffer library call is passing in the base type, but if the instance is actually a derived type, more data than expected will be written. In the sample above 12 bytes are written into an 8-byte buffer.

I couldn't tell you why 2.0 doesn't provide a generic overload (or an overload containing a System.Type parameter) to resolve this. Outside the generic environment, one can make an encapsulation class with a single field of the expected element type to "trick" the marshaler into marshaling only the base type even if the instance is derived. Unfortunately, classes with generic reference-type fields cannot be marshaled so in the sample above the only realistic solution is to fail (throw an exception) if the element instance type is not the expected type. Sure one can write each element into a size-checked buffer before copying the base size over to the destination buffer, but that is a lot of undesirable work for something which should be simple. Even that work-around won't work if the derived type is not marshalable.

Perhaps I overlooked a work-around, let me know if I did. :-)

Published Saturday, August 13, 2005 1:10 AM by devinj

Comment Notification

If you would like to receive an email when updates are made to this post, please register here

Subscribe to this post's comments using RSS

Comments

 

Christopher Steen said:

[MAJOR RELEASE] WSCF 0.51 [Via:
Christian Weyer ]
Awesome security content for ASP.NET 2.0 -- a bunch...
August 13, 2005 11:52 PM
 

Christopher Steen - Learning .NET said:

[MAJOR RELEASE] WSCF 0.51 [Via: Christian Weyer ]
Awesome security content for ASP.NET 2.0 -- a bunch...
August 13, 2005 11:53 PM
 

Shaun Bedingfield said:

I must agree that the functionality presented here is illogical and likely to get many programmers in trouble. I hope Microsoft makes these two functions more consistent in the future or this is likely to cause a lot of memory leaks.

I searched the knowledgebase and MSDN libary looking for work-arounds. I am left with only one untested idea.. custom serialization.
August 14, 2005 12:48 AM
 

Jonathan Porter said:

Solution is to constrain T to a struct.

With:

where T: struct

January 11, 2008 4:27 PM
 

Computer Help said:

Really great information, has given me an idea for a blog of my friends.

October 15, 2008 4:49 PM
 

Hugh said:

With a recoding of the algorithm to organize memory a little differently, this can be made safe, the key step is to compute the length using:

Marshal.SizeOf(element.GetType())

This does not fall victim to the issue you cite, which is is very important incidentally, of course your algorithm needs adjusting to handle variable length elements, but at least you can always get the correct size for any element.

December 22, 2008 8:37 AM

Leave a Comment

(required) 
(optional)
(required) 
Submit

© 2009 Microsoft Corporation. All rights reserved. Terms of Use  |  Trademarks  |  Privacy Statement
Microsoft
Page view tracker