Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory corruption in Mono 5.8.0.108. #6777

Closed
dlemstra opened this issue Feb 4, 2018 · 17 comments

Comments

Projects
None yet
6 participants
@dlemstra
Copy link

commented Feb 4, 2018

Steps to Reproduce

In one of the ImageSharp issues (SixLabors/ImageSharp#439) we got a report about incorrect loading of images with Mono. After some investigation we tracked it down to a method that swaps two buffers. The swap corrupted one of the fields in the class and caused the image to load incorrectly. Below is a small sample program that can be used to reproduce the issue.

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

public class Program
{
    internal class TestBuffer
    {
        private IntPtr pointer;
        private GCHandle handle;
        private bool boolean;

        public TestBuffer(int length)
        {
            this.Length = length;
        }

        public int Length { get; private set; }
    }

    internal class Test
    {
        private int counter;
        private TestBuffer buffer1;
        private TestBuffer buffer2;

        public Test()
        {
            this.counter = 42;
            this.buffer1 = new TestBuffer(4);
            this.buffer2 = new TestBuffer(2);
        }

        public void SwapThatTriggersCorruption()
        {
            SwapThatTriggersCorruption(ref this.buffer2, ref this.buffer1);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private static void SwapThatTriggersCorruption<T>(ref T first, ref T second)
        {
            T temp = second;
            second = first;
            first = temp;
        }

        public void SwapThatWorks()
        {
            SwapThatWorks(ref this.buffer2, ref this.buffer1);
        }

        private static void SwapThatWorks<T>(ref T first, ref T second)
        {
            T temp = second;
            second = first;
            first = temp;
        }

        public int Counter => this.counter;
    }

    public static void Main(string[] args)
    {
        var test = new Test();
        Console.WriteLine(test.Counter);
        test.SwapThatTriggersCorruption();
        //test.SwapThatWorks();
        Console.WriteLine(test.Counter);
    }
}

Current Behavior

The value of test.Counter gets corrupted and this results in the following output:

mono@Mono:~/Mono$ mono Sample.exe
42
31178024
mono@Mono:~/Mono$ mono Sample.exe
42
18161304

Expected Behavior

It would be nice if the Aggressive Swap would stop touching the counter field.

mono@Mono:~/Mono$ mono Sample.exe
42
42
mono@Mono:~/Mono$ mono Sample.exe
42
42

On which platforms did you notice this

The problem can be reproduced on Linux.

mono@Mono:~/Mono$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 16.04.3 LTS
Release:        16.04
Codename:       xenial

The problem cannot be reproduced on Windows and I don't own a Mac so I could not test it there.

Version Used:

mono@Mono:~/Mono$ mono -V
Mono JIT compiler version 5.8.0.108 (tarball Fri Jan 19 18:15:21 UTC 2018)
Copyright (C) 2002-2014 Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
        TLS:           __thread
        SIGSEGV:       altstack
        Notifications: epoll
        Architecture:  amd64
        Disabled:      none
        Misc:          softdebug
        LLVM:          supported, not enabled.
        GC:            sgen (concurrent by default)
@jaykrell

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2018

This reproduces with current code (Mac/amd64).

@jaykrell

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2018

This is great bug report by the way. Here it is slightly reduced.
Now to study the output of mono -v -v w/ and w/o the aggressive inline attribute.

using System.Runtime.CompilerServices;

public class Program
{
	int counter = 42;
	Program buffer1;
	Program buffer2;
	static volatile int line;

	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	private static void SwapThatTriggersCorruption<T>(ref T first, ref T second, ref int line)
	{
		line = 3;
		second = first;
		line = 4;
	}

	public void Main()
	{
		line = 1;
		Console.WriteLine(counter);
		line = 2;
		SwapThatTriggersCorruption(ref buffer2, ref buffer1, ref line);
		line = 5;
		Console.WriteLine(counter);
		line = 6;
	}

	public static void Main(string[] args)
	{
		new Program().Main();
	}
}
@jaykrell

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2018

Heh, this smaller one reproduces without the attribute too.

@jaykrell

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2018

(try --optimize=-inline)

@dlemstra

This comment has been minimized.

Copy link
Author

commented Feb 4, 2018

I can from a much bigger code sample but I tried to trim it down and see where it breaks. Great that you could trim it down even more. I can also reproduce it with your code without the attribute. Maybe the compiler decides to inline it themselves? Running the executable that was compiled on my Linux machine on Windows returns the expected result.

I can also reproduce it without the line being volatile.

@Therzok

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

I can also reproduce it with your code without the attribute. Maybe the compiler decides to inline it themselves?

Yup, Swap has code size == 19, so it's a candidate for inlining.

ikdasm on the .exe shows this:

  .method private hidebysig static void  SwapThatTriggersCorruption<T>(!!T& first,
                                                                       !!T& second,
                                                                       int32& line) cil managed aggressiveinlining
  {
    // Code size       19 (0x13)
...
@jaykrell

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2018

It is specific to generics btw.

using System.Runtime.CompilerServices;

public class Program
{
    int counter = 42;
    Program buffer1;
    Program buffer2;
    static volatile int line;

    // works
    private static void F3(ref Program first, ref Program second, ref int line)
    // does not work
    //private static void F3<Program>(ref Program first, ref Program second, ref int line)
    {
        line = 3;
        second = first;
        line = 4;
    }

    public void F2()
    {
        line = 1;
        Console.WriteLine(counter);
        line = 2;
        F3(ref buffer2, ref buffer1, ref line);
        line = 5;
        Console.WriteLine(counter);
        line = 6;
    }

    public static void Main(string[] args)
    {
        new Program().F2();
    }
}
@dlemstra

This comment has been minimized.

Copy link
Author

commented Feb 4, 2018

For future reference: Should I have reported this somewhere else first due to the possible security implications of this issue?

@vargaz

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

This was broken by e05343d, which removed the !ref check from ldobj+stobj:

  •                   if (((ip [5] == CEE_STOBJ) && ip_in_bb (cfg, cfg->cbb, ip + 5) && read32 (ip + 6) == token) && !generic
    

_class_is_reference_type (cfg, klass) && !(ins_flag & MONO_INST_VOLATILE)) {

  •                   if (((ip [5] == CEE_STOBJ) && ip_in_bb (cfg, cfg->cbb, ip + 5) && read32 (ip + 6) == token)) {
    
@vargaz

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

Mono is not a sandbox, so this does not qualify as a security issue.

vargaz added a commit to vargaz/mono that referenced this issue Feb 4, 2018

monojenkins added a commit to monojenkins/mono that referenced this issue Feb 4, 2018

@marek-safar marek-safar added this to the 2017-12 milestone Feb 4, 2018

marek-safar added a commit that referenced this issue Feb 4, 2018

marek-safar added a commit that referenced this issue Feb 5, 2018

@vargaz

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

This one is in 2017-10 too, so it needs backport to 2017-10 and the products.

@vargaz vargaz reopened this Feb 5, 2018

monojenkins added a commit to monojenkins/mono that referenced this issue Feb 5, 2018

@marek-safar marek-safar modified the milestones: 2017-12, 2017-10 Feb 5, 2018

@brendanzagaeski

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

Would it be accurate to say that this issue could in theory cause incorrect data to be saved, which would count as a kind of data loss? For example, if an app loads a user file, and then one of the file editing steps uses a generic method that hits this issue, could that result in data loss when the user re-saves the file?

Thanks in advance!

@brendanzagaeski

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

I will follow-up on my question about potential data loss with a little local testing to satisfy my curiosity. I will plan to complete that shortly and reply with the results.

@brendanzagaeski

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

Results of severity sanity checking:

  • Yes, this bug can lead to silent data loss. This means that some affected users might not have noticed the problem yet.
  • Yes, it affects both Xamarin.Android and Xamarin.iOS starting with their d15-5 release branches (which reference the Mono 2017-06 branch). And yes, it affects Release builds (on both emulators and devices).
  • The [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute is not strictly required to hit the issue. I removed it when testing on iOS and Android. This means the circumstances required to hit the issue are even a bit less specific and so there's a higher chance that existing users code bases are affected.
@brendanzagaeski

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2018

Non-public work items to track inclusion into Visual Studio 2017:

jonpryor added a commit to xamarin/xamarin-android that referenced this issue Feb 8, 2018

@marek-safar

This comment has been minimized.

Copy link
Member

commented Feb 15, 2018

@brendanzagaeski are you ok with closing the issue

@brendanzagaeski

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

Yup, closing it sounds fine to me.

joncham added a commit to Unity-Technologies/mono that referenced this issue Apr 6, 2018

jonpryor added a commit to xamarin/xamarin-android that referenced this issue Apr 25, 2018

Bump to mono/2018-02/0c5a524e (#1289)
Bumps to Java.Interop/master/0afb2b0f
Bumps to llvm/master/a9cfb50e.

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=11771
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=15051
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=19436
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=45901
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=56071
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=58413
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=58413
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=58413
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=59184
fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60065
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60225
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60298
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60359
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60568
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60756
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60848
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60862
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60900
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60904
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60986
Fixes: https://github.com/mono/mono/issues/59400
Fixes: mono/mono#6169
Fixes: mono/mono#6187
Fixes: mono/mono#6192
Fixes: mono/mono#6255
Fixes: mono/mono#6264
Fixes: mono/mono#6266
Fixes: mono/mono#6281
Fixes: mono/mono#6283
Fixes: mono/mono#6320
Fixes: mono/mono#6339
Fixes: mono/mono#6343
Fixes: mono/mono#6349
Fixes: mono/mono#6379
Fixes: mono/mono#6383
Fixes: mono/mono#6401.
Fixes: mono/mono#6411
Fixes: mono/mono#6414
Fixes: mono/mono#6490
Fixes: mono/mono#6721
Fixes: mono/mono#6767
Fixes: mono/mono#6777
Fixes: mono/mono#6848
Fixes: mono/mono#6940
Fixes: mono/mono#6948
Fixes: mono/mono#6998
Fixes: mono/mono#7016
Fixes: mono/mono#7085
Fixes: mono/mono#7086
Fixes: mono/mono#7095
Fixes: mono/mono#7137
Fixes: mono/mono#7184
Fixes: mono/mono#7240
Fixes: mono/mono#7262
Fixes: mono/mono#7289
Fixes: mono/mono#7338
Fixes: mono/mono#7356
Fixes: mono/mono#7364
Fixes: mono/mono#7378
Fixes: mono/mono#7389
Fixes: mono/mono#7460
Fixes: mono/mono#7535
Fixes: mono/mono#7536
Fixes: mono/mono#7610
Fixes: mono/mono#7624
Fixes: mono/mono#7637
Fixes: mono/mono#7655
Fixes: mono/mono#7657
Fixes: mono/mono#7685
Fixes: mono/mono#7786
Fixes: mono/mono#7792
Fixes: mono/mono#7822
Fixes: mono/mono#7860
Fixes: mono/mono#8089
Fixes: mono/mono#8267
Fixes: mono/mono#8409
Fixes: xamarin/maccore#628
Fixes: xamarin/maccore#629
Fixes: xamarin/maccore#673
Fixes: xamarin/maccore#673
Fixes: #1561
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.