[jni] On Windows, look for side-by-side libmonosgen. Fix #53163 #496

garuma · 2017-03-15T20:23:49Z

Context: https://bugzilla.xamarin.com/show_bug.cgi?id=53163

A user uncovered an issue using the Xamarin.Forms previewer where libmonosgen failed to load correctly on Windows.

The root cause was that the already loaded libmono-android was incompatible with the version of mono being loaded (different bridge versions) causing a forced runtime shutdown with the error message:

Invalid bridge callback version. Expected 4 but got 5

After investigating, it turns out the user had an old version of Xamarin installed via MSI (for VS2015) on his machine and was trying to use the newer Xamarin embedded in VS2017 via the Willow distribution mechanism which places binaries such as libmonosgen in a separate directory structure.

Thus the problem was that, while libmono-android itself was loaded correctly from that separate path, the code was then trying to fetch libmonosgen from the global system path (now completely obsolete) causing the bridge version mismatch.

With this patch, the code will now try first to get libmonosgen from the same directory libmono-android was loaded from which should cover the Willow case.

Context: https://bugzilla.xamarin.com/show_bug.cgi?id=53163 A user uncovered an issue using the Xamarin.Forms previewer where libmonosgen failed to load correctly on Windows. The root cause was that the already loaded libmono-android was incompatible with the version of mono being loaded (different bridge versions) causing a forced runtime shutdown with the error message: Invalid bridge callback version. Expected 4 but got 5 After investigating, it turns out the user had an old version of Xamarin installed via MSI (for VS2015) on his machine and was trying to use the newer Xamarin embedded in VS2017 via the Willow distribution mechanism which places binaries such as libmonosgen in a separate directory structure. Thus the problem was that, while libmono-android itself was loaded correctly from that separate path, the code was then trying to fetch libmonosgen from the global system path (now completely obsolete) causing the bridge version mismatch. With this patch, the code will now try first to get libmonosgen from the same directory libmono-android was loaded from which should cover the Willow case.

jonpryor · 2017-03-16T00:51:28Z

src/monodroid/jni/monodroid-glue.c

+		return libmonoandroid_directory_path;
+
+	if (!GetModuleHandleEx (GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT,
+	                        (LPCSTR)&libmonoandroid_directory_path, &module))


Your editor is using way too many spaces. This should be three TABs, not a bazillion spaces.

jonpryor · 2017-03-16T01:02:07Z

src/monodroid/jni/monodroid-glue.c

+	if (libmonoandroid_directory_path != NULL)
+		return libmonoandroid_directory_path;
+
+	if (!GetModuleHandleEx (GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT,


I don't think that this has explicitly come up before, but in general, we should follow the precepts in UTF-8 Everywhere, especially the Windows guidelines:

Do not use types, functions, or their derivatives that are sensitive to the UNICODE constant, such as LPTSTR, CreateWindow() or the _T() macro. Instead, use LPWSTR, CreateWindowW() and explicit L"" literals.

Only use Win32 functions that accept widechars (LPWSTR), never those which accept LPTSTR or LPSTR.

Consequently, this should use GetModuleHandleExW() explicitly, not GetModuleHandleEx(). Passing & libmonoandroid_directory_path is also really suspect to my eyes; it may work, but...ugh.

Instead, I'd want to declare at global scope:

static WCHAR for_address_computation;

Then we don't need a cast:

if (!GetModuleHandleExW ( GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, &for_address_computation, &module)) return NULL;

When called with GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS the pointer is equivalent to a void*, being Unicode/ANSI doesn't matter here. The cast is meaningless and is just there to prevent a compiler warning

Changed the cast to be void* instead so that it's clearer we are not using this as a string parameter

jonpryor · 2017-03-16T01:02:32Z

src/monodroid/jni/monodroid-glue.c

+	if (!GetModuleHandleEx (GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT,
+	                        (LPCSTR)&libmonoandroid_directory_path, &module))
+		return NULL;
+	GetModuleFileNameW (module, module_path, sizeof (module_path));


See? Here you use GetModuleFileNameW(), as is good and proper. :-)

sizeof (module_path) is wrong here. According to the GetModuleFileName() docs:

nSize [in]

The size of the lpFilename buffer, in TCHARs.

Alas, this is not sizeof(module_path). This is instead sizeof(module_path)/sizeof(module_path[0]).

Aside: this is the first time I've ever even heard of the _pgmptr global variable, encountered from reading those docs...

jonpryor · 2017-03-16T01:20:51Z

src/monodroid/jni/monodroid-glue.c

+		return NULL;
+	GetModuleFileNameW (module, module_path, sizeof (module_path));
+	converted_path = utf16_to_utf8 (module_path);
+	libmonoandroid_directory_path = dirname (converted_path);


Does Windows provide a dirname() function? I can't find documentation for it.

I see mingw source code for dirname(), but I don't think we link against any mingw libraries -- do we? Should we?

I suspect the easier path would be to use strrchr(), akin to:

*strrchr (converted_path, '\\') = '\0'; libmonoandroid_directory_path = converted_path;

The primary reason I ask is because I don't know if dirname() allocates a new string or not, and I'm thinking about memory leaks.

The macOS dirname(3) man page is horrifying:

The dirname() function returns a pointer to internal storage space allocated on the first call that will be overwritten by subsequent calls. dirname_r() is therefore preferred for threaded applications.

Other vendor implementations of dirname() may modify the contents of the string passed to dirname(); if portability is desired, this should be taken into account when writing code which calls this function.

Both of those notes scream "flee in terror" to me...

dirname is implemented by the MinGW CRT, it seems more robust to me than just strchr it. The equivalent Windows method requires to link in another library, I initially planned to avoid it but I can use that instead if you want.

How is the strrchr() not robust? We know that GetModuleFileNameW() is going to return \\ as the directory separator char. (The mingw source supports / as well, which shouldn't be necessary here.)

So I guess since we both dislike dirname and I don't feel good with strchr we can go for either PathRemoveFileSpec or PathCchRemoveFileSpec. The first is deprecated in favor of the second but that second one is only on Win 8+, do we still have people using our stuff in Windows 7? Could also use both with #ifdef'ing

I think using PathRemoveFileSpec() should be fine. VS2017 runs on Windows 7 SP1 and later.

… type doesn't matter

jonpryor · 2017-03-16T19:25:46Z

src/monodroid/jni/monodroid-glue.c

+		return libmonoandroid_directory_path;
+
+	DWORD flags = GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT;
+	if (!GetModuleHandleEx (flags, (void*)&libmonoandroid_directory_path, &module))


This should still be using GetModuleHandleExW(). :-)

…of module

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=53163 A user uncovered an issue using the Xamarin.Forms previewer where libmonosgen failed to load correctly on Windows. The root cause was that the already loaded libmono-android was incompatible with the version of mono being loaded (different bridge versions) causing a forced runtime shutdown with the error message: Invalid bridge callback version. Expected 4 but got 5 After investigating, it turns out the user had an old version of Xamarin installed via MSI (for VS2015) on his machine and was trying to use the newer Xamarin embedded in VS2017 via the Willow distribution mechanism which places binaries such as libmonosgen in a separate directory structure. Thus the problem was that, while libmono-android itself was loaded correctly from that separate path, the code was then trying to fetch libmonosgen from the global system path (now completely obsolete) causing the bridge version mismatch. With this patch, the code will now try first to get libmonosgen from the same directory libmono-android was loaded from which should cover the Willow case.

dnfclas added the cla-already-signed label Mar 15, 2017

garuma assigned jonpryor Mar 15, 2017

jonpryor reviewed Mar 16, 2017

View reviewed changes

garuma added 3 commits March 16, 2017 11:06

[jni] Fix method call parameters alignment

14d852a

[jni] Pass correct size for GetModuleFileNameW

b92256f

[jni] Cast to void* instead of LPCSTR to make it clear the string…

3ec0632

… type doesn't matter

jonpryor reviewed Mar 16, 2017

View reviewed changes

garuma added 2 commits March 16, 2017 15:29

[jni] GetModuleHandleEx -> GetModuleHandleExW

6f19c28

[jni] Use PathRemoveFileSpec instead of dirname to compute directory …

f1e91fc

…of module

jonpryor merged commit 8d7cc2b into master Mar 17, 2017

jonpryor deleted the designer-checkforalongsidemono branch June 11, 2019 21:46

github-actions bot locked and limited conversation to collaborators Feb 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jni] On Windows, look for side-by-side libmonosgen. Fix #53163 #496

[jni] On Windows, look for side-by-side libmonosgen. Fix #53163 #496

Uh oh!

garuma commented Mar 15, 2017 •

edited

Loading

Uh oh!

jonpryor Mar 16, 2017

Uh oh!

jonpryor Mar 16, 2017

Uh oh!

garuma Mar 16, 2017

Uh oh!

garuma Mar 16, 2017

Uh oh!

jonpryor Mar 16, 2017

Uh oh!

jonpryor Mar 16, 2017

Uh oh!

jonpryor Mar 16, 2017

Uh oh!

garuma Mar 16, 2017

Uh oh!

jonpryor Mar 16, 2017

Uh oh!

garuma Mar 16, 2017 •

edited

Loading

Uh oh!

jonpryor Mar 16, 2017

Uh oh!

jonpryor Mar 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[jni] On Windows, look for side-by-side libmonosgen. Fix #53163 #496

[jni] On Windows, look for side-by-side libmonosgen. Fix #53163 #496

Uh oh!

Conversation

garuma commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garuma Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

garuma commented Mar 15, 2017 •

edited

Loading

garuma Mar 16, 2017 •

edited

Loading