Bug 58432 - Marshaling string field with MarshalAs(UnmanagedType.LPStr) attribute is broken on iOS 11 beta device
Summary: Marshaling string field with MarshalAs(UnmanagedType.LPStr) attribute is brok...
Status: RESOLVED NOT_ON_ROADMAP
Alias: None
Product: iOS
Classification: Xamarin
Component: Mono runtime / AOT compiler ()
Version: XI 10.99 (xcode9)
Hardware: PC Windows
: --- normal
Target Milestone: Untriaged
Assignee: Zoltan Varga
URL:
Depends on:
Blocks:
 
Reported: 2017-07-27 12:08 UTC by Oleg
Modified: 2017-09-18 15:40 UTC (History)
4 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Repro of stream error behavior (11.80 KB, application/x-zip-compressed)
2017-08-07 08:54 UTC, Oleg
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED NOT_ON_ROADMAP

Description Oleg 2017-07-27 12:08:36 UTC
We have the following struct:

    [StructLayout(LayoutKind.Sequential)]
    internal struct ZStream
    {
		public IntPtr next_in;          /* next input byte */
		public _uint avail_in;           /* number of bytes available at next_in */
		public _uint total_in;           /* total nb of input bytes read so far */
		public IntPtr next_out;         /* next output byte should be put there */
		public _uint avail_out;          /* remaining free space at next_out */
		public _uint total_out;          /* total nb of bytes output so far */
		[MarshalAs(UnmanagedType.LPStr)]
		public string msg;              /* last error message, NULL if no error */
		private IntPtr state;           /* not visible by applications */
		private IntPtr zalloc;          /* used to allocate the internal state */
		private IntPtr zfree;           /* used to free the internal state */
		private IntPtr opaque;          /* private data object passed to zalloc and zfree */
		public DataType data_type;      /* best guess about the data type: ascii or binary */
		public _uint adler;              /* adler32 value of the uncompressed data */
		private _uint reserved;          /* reserved for future use */
    }

and the following P/Invoke method declared:

        [DllImport("__Internal", CallingConvention = CallingConvention.Cdecl, EntryPoint = "inflateInit2_")]
        private static extern ReturnCode InflateInit2_(ref ZStream strm,
                                                      WindowBits windowBits,
                                                      [MarshalAs(UnmanagedType.LPStr)] string version,
                                                      _int zstreamSize);


we invoke it like this:

         ZStream _zStream;
         InflateInit2_(ref _zStream, windowBits, ZLibVersion, Marshal.SizeOf(typeof(ZStream)));

This reproduced on any production build with previous Xamarin iOS SDK as less as with the latest one with XCode 9 beta 4.

Changing struct field "msg" to the following declaration fixes the issue:

[StructLayout(LayoutKind.Sequential)]
    internal struct ZStream
    {
		public IntPtr next_in;          /* next input byte */
		public _uint avail_in;           /* number of bytes available at next_in */
		public _uint total_in;           /* total nb of input bytes read so far */
		public IntPtr next_out;         /* next output byte should be put there */
		public _uint avail_out;          /* remaining free space at next_out */
		public _uint total_out;          /* total nb of bytes output so far */
     -->        public IntPtr msg;              /* last error message, NULL if no error */
		private IntPtr state;           /* not visible by applications */
		private IntPtr zalloc;          /* used to allocate the internal state */
		private IntPtr zfree;           /* used to free the internal state */
		private IntPtr opaque;          /* private data object passed to zalloc and zfree */
		public DataType data_type;      /* best guess about the data type: ascii or binary */
		public _uint adler;              /* adler32 value of the uncompressed data */
		private _uint reserved;          /* reserved for future use */
    }

It would be great to have this fix in the closest upcoming stable release of Xamarin iOS SDK, not only 10.99 as it affects production apps.
Comment 1 Zoltan Varga 2017-07-27 18:12:03 UTC
Whats the failure ? Does it crash ? Is there a stack trace ?
Comment 2 Zoltan Varga 2017-07-27 18:32:41 UTC
According to .net marshalling rules, the 'msg' field received from native code will be freed, because its assumed to be dynamically allocated by the native code.
However, zlib doesn't do this, from

https://www.zlib.net/manual.html

"In the error case, msg may be set but then points to a static string (which must not be deallocated)."

So the original declaration is not going to work, the IntPtr msg version should be used.
Comment 3 Oleg 2017-07-27 18:59:24 UTC
Error we get is -2 (stream error).
The thing is this code was like this for more than 3 years and lived well will a lot of versions of Xamarin and on iOS from 6 to 10 and still works everywhere except iOS 11 devices.

As I said we have got it working with change of "msg" field, but it would be nice to have it fixed or got definite answer that it is iOS 11 behavior you cannot change.
Comment 4 Zoltan Varga 2017-07-27 22:42:52 UTC
I believe this only worked by accident, and some ios11 changes caused the issue to surface. Declaring the 'msg' field as a string is not good, it doesn't conform to .net marshalling rules.
Comment 5 Grigory (Playtika) 2017-07-31 16:03:24 UTC
Hi Zoltan,

>> According to .net marshalling rules, the 'msg' field received from native code will be freed,

This is a false statement. There is no such rule, as it makes no sense.
During marshalling .NET copies chars from managed string to pinned buffer, and passes IntPtr to unmanaged code. When unmanaged code returns some ptr, .NET copies chars from this ptr to a new string AND frees the buffer that it created BEFORE passing control to unmanaged code.

This is a normal flow that worked for years in .NET.
Comment 7 Grigory (Playtika) 2017-07-31 16:30:01 UTC
In any case, issue is that structure is marshalled improperly, and zlib receives broken structure. 

The other data is broken, BECAUSE of error in string marshalling.

String is null both in IN and OUT structure, we don't care about its value.
Comment 8 Manuel de la Peña [MSFT] 2017-08-02 15:58:45 UTC
@Zoltan, I'm setting the bug to assigned so that we keep our bug list under control.
Comment 9 Zoltan Varga 2017-08-07 05:22:04 UTC
I can't reproduce this. Note that in the above structure, the 'total_in', 'total_out', 'adler' and 'reserved' fields are defined as uLong in c:
https://www.zlib.net/manual.html

uLong is the same as C long type, which has no easy mapping to c#. The best mapping is to define these fields as IntPtr, which will work on every os except 64 bit windows, where pointers are 64 bit but long is still 32 bit.
Comment 10 Oleg 2017-08-07 08:54:05 UTC
Created attachment 24048 [details]
Repro of stream error behavior

Here is a sample reproducing wrong behaviour. As is it should run with error on iOS11 only. Then when you change ZStream.msg declaration in ZLibBinding.cs everything works as expected.
Comment 11 Zoltan Varga 2017-08-07 18:32:30 UTC
I can reproduce using the testcase.
Comment 12 Zoltan Varga 2017-08-07 20:59:16 UTC
This seems to be caused by the addition of a reference field making the structure 'non-blittable', i.e.
adding a
		[MarshalAs (UnmanagedType.ByValArray, SizeConst=2)] public short[] a1;
field at the end of the structure also causes the same error.

ZStream being non-blittable is not good for performance, but it should still work.
Comment 13 Grigory (Playtika) 2017-09-15 14:40:24 UTC
Hi Zoltan, any progress on that ?
Comment 14 Zoltan Varga 2017-09-15 15:38:19 UTC
No this was given lower priority since there is a workaround.
Comment 15 Zoltan Varga 2017-09-18 15:40:14 UTC
This is not a xamarin.ios bug. zlib expects to be passed the same z_stream pointer to its functions, i.e. deflateInit () and deflate () should be passed the same z_stream argument. If the ZStream managed structure contains a string field, the runtime will have to perform marshalling, i.e. it will create a copy of the structure and pass a pointer to that to the native function. So the two functions will receive different pointers.

This check was recently added to zlib:

https://github.com/madler/zlib/commit/b516b4bdd7c0c9f0858adfebf732089014f7b282

ios11/xcode probably ships this updated zlib version.