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

StringBuilder marshalling bug #15366

Closed
EgorBo opened this issue Jun 23, 2019 · 6 comments · Fixed by #15392 or xamarin/xamarin-android#3352

Comments

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Jun 23, 2019

Steps to Reproduce

  1. Compile:
using System;
using System.Runtime.InteropServices;
using System.Text;

class Program
{
    [DllImport("user32.dll", CharSet = CharSet.Unicode)]
    static extern uint CharLowerBuffW([In, Out] StringBuilder lpsz, uint cchLength);

    static void Main()
    {
        var sb = new StringBuilder().AppendLine("HELLO").AppendLine("WORLD").AppendLine("FROM MONO");
        CharLowerBuffW(sb, (uint)sb.Length);
        Console.WriteLine(sb); // sb is corrupted/freed at this step
    }
}

Expected Behavior

hello
world
from mono

Actual Behavior

Segfault
or sometimes StringBuilder contains some random chars at the end after marshalling.

On which platforms did you notice this

[ ] macOS
[ ] Linux
[x] Windows

@filipnavara

This comment has been minimized.

Copy link
Collaborator

@filipnavara filipnavara commented Jun 23, 2019

There are two separate bugs triggered by this and one more bug related to it:

  1. mono_string_builder_to_utf16_impl doesn't properly NULL terminate the strings. It appends a terminating zero at the location specified by capacity, not by length. Workaround: Add str [mono_string_builder_string_length (sb)] = 0;.
  2. mono_string_utf16_to_builder_copy corrupts memory when trying to marshal data back into multi-chunk StringBuilder. Adding g_assert (mono_array_handle_length (chunkChars) >= string_len); to it helps tracking down to a place where the memory corruption actually happens.
  3. Even when the StringBuilder is marshalled only with the [In] attribute the code in 2) is still triggered and causes memory corruption.
@filipnavara

This comment has been minimized.

Copy link
Collaborator

@filipnavara filipnavara commented Jun 23, 2019

Confirmed to be the underlying cause of #15306. Big thanks for tracking it down!

@steveisok steveisok changed the title StringBuilder marshalling bug [netcore] StringBuilder marshalling bug Jun 23, 2019
@EgorBo

This comment has been minimized.

Copy link
Member Author

@EgorBo EgorBo commented Jun 23, 2019

@steveisok btw, this issue reproduces on regular mono too 🙂

@filipnavara

This comment has been minimized.

Copy link
Collaborator

@filipnavara filipnavara commented Jun 23, 2019

Yep, affects regular Mono too. I have a fix for some of the issues in the pipeline but I need to clean it up before submitting.

@steveisok steveisok changed the title [netcore] StringBuilder marshalling bug StringBuilder marshalling bug Jun 23, 2019
@steveisok

This comment has been minimized.

Copy link
Contributor

@steveisok steveisok commented Jun 23, 2019

Thanks, labeled (I think) correctly.

@filipnavara

This comment has been minimized.

Copy link
Collaborator

@filipnavara filipnavara commented Jun 24, 2019

@marek-safar While it is most visible on Windows it affects all systems. Bug 1 is UTF-16 specific, bug 2 happens with UTF-8 too AFAICT, bug 3 affects everything.

@marek-safar marek-safar added this to the 2019-06 (6.4.xx) milestone Jul 15, 2019
akoeplinger added a commit that referenced this issue Jul 15, 2019
Fixes #15366 and #15306.

- Fixes NULL termination of `StringBuilder` to UTF-16 marshalling. Additionally an extra NULL is added as defense-in-depth protection for incorrect handling in native calls. This extra NULL adds protection when trying to marshal the NULL-terminated string back and the native function doesn't null-terminate at the size of the buffer.
- Fixes marshalling back from UTF-16/UTF-8 buffers into chunked `StringBuilder`.
- Fixes missing `mono_marshal_free` call causing a memory leak when marshalling `StringBuilder` to UTF-16 and back.
- Skips marshalling back to `StringBuilder` when the parameter doesn't have `[Out]` attribute.

Unit tests are still TBD. Not sure where to put them and how to structure them. Any help would be appreciated. /cc @vargaz @EgorBo 

Backport of #15392.
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Jul 16, 2019
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Jul 17, 2019
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Jul 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.