Replace dlfcn-win32 with simple-dlfcn-win32 #226

mcnameej · 2015-08-07T23:18:48Z

I'm submitting two PR's to fix #223. Pick one. This PR replaces the dlfcn-win32 code.

unbornchikken · 2015-08-08T05:42:06Z

I'd go with this definitely, but in this case, we don't need this to reside in deps. Could you integrate it with src?

Thanks a lot!

(2) Convert filename from UTF-8 to UCS-2 and use LibraryLibraryW(). (3) Code clean up and hardening.

mcnameej · 2015-08-08T16:41:45Z

Three test cases fail with my version of dlfcn...

1) Library should accept `null` as a first argument:
   Error: Dynamic Symbol Retrieval Error: Win32 error 127
    at DynamicLibrary.get (C:\Proj\iojs\node-ffi\lib\dynamic_library.js:112:11)
    at C:\Proj\iojs\node-ffi\lib\library.js:50:19
    at Array.forEach (native)
    at new Library (C:\Proj\iojs\node-ffi\lib\library.js:47:28)
    at Context.<anonymous> (C:\Proj\iojs\node-ffi\test\library.js:24:21)

2) Library should work with "strcpy" and a 128 length string:
   Error: Dynamic Symbol Retrieval Error: Win32 error 127
    at DynamicLibrary.get (C:\Proj\iojs\node-ffi\lib\dynamic_library.js:112:11)
    at C:\Proj\iojs\node-ffi\lib\library.js:50:19
    at Array.forEach (native)
    at new Library (C:\Proj\iojs\node-ffi\lib\library.js:47:28)
    at Context.<anonymous> (C:\Proj\iojs\node-ffi\test\library.js:64:18)

3) Library should work with "strcpy" and a 2k length string:
   Error: Dynamic Symbol Retrieval Error: Win32 error 127
    at DynamicLibrary.get (C:\Proj\iojs\node-ffi\lib\dynamic_library.js:112:11)
    at C:\Proj\iojs\node-ffi\lib\library.js:50:19
    at Array.forEach (native)
    at new Library (C:\Proj\iojs\node-ffi\lib\library.js:47:28)
    at Context.<anonymous> (C:\Proj\iojs\node-ffi\test\library.js:74:18)

These test cases depend on POSIX semantics in dlsym: searching all loaded modules to find a symbol by name. No Windows programmer would ever think to do that.

When you search for "strcpy", which strcpy() do you expect to find? ntdll.dll is loaded into all processes, and it exports strcpy. msvcrt.dll is loaded into most processes, but it belongs to the OS -- node.exe doesn't use it. In fact, node.exe static links it's C runtime, and those symbols are unavailable (even the original dlfcn-win32, which tries to emulate POSIX, can't see them). If node.exe did use a C runtime DLL, it would be something like msvcr120.dll. You have several DLL's loaded in your process that export "strcpy". Which one will dlsym return? Who knows.

It probably doesn't matter which runtime you get strcpy() from. It matters a lot for things like malloc(), free(), fopen(), and fclose(). Memory allocated by malloc() must be passed to the free() in the same C runtime as the malloc() that allocated it. The FILE* returned by fopen() can only be passed to stdio functions in the same C runtime. This is why Windows DLL's generally don't use stdio FILE's in their public interface, and if they have functions that return an allocated buffer, they'll provide a function to free it (or they'll allocate the memory using a Win32 API like GlobalAlloc() or HeapAlloc(), which any caller can free).

unbornchikken · 2015-08-08T17:10:37Z

I understand your point, and agree. I think those test are not there to test strcmp, those are there to test if binding is works or not by the given situation. If you could replace them with an invocations of an strcmp wrapper method in ffi_test.node, then I'd accept it that way. That node file is just a dynamic lib, so it gotta work that way. @TooTallNate waddaya say?

(1) Avoid depending on POSIX dynamic library semantics. (2) Check handling of Unicode characters in path/filename.

mcnameej · 2015-08-08T21:11:18Z

P.S. My last commit included a Unicode test case similar to PR #225.

unbornchikken · 2015-08-10T06:49:58Z

So to sum it up:

finally we can drop another dependency, only ffi remains
we have a simplifed dlopen and co. on Windows that also fixes Dll pathname with non-ascii characters #183
there are some updated test cases of testing the same node-ffi behavior, but wouldn't encourage anyone to use node-ffi in a way that never really works right on Windows

@TooTallNate that's what I call pull request. Could be merged ASAP. Agree?

unbornchikken · 2015-08-10T06:58:53Z

I celebrated too early. Unfortunattely we have some failing test cases by merging this on Linux. Please take a look at the Travis log. Do you have experience to fix them? If not, I can help you with this.

mcnameej · 2015-08-10T13:31:26Z

I'll fire up a Linux VM and investigate the failures reported by travis.

Not sure what to make of the failures reported by AppVeyor. Is that a problem with the patch, or a problem with the environment?

unbornchikken · 2015-08-10T13:47:12Z

You can ignore AppVeyor's log, thats because of iojs.exe instead of node.exe issue. Travis should work with 0.10 and 0.12. With io.js it will fail because 3.x support is blocked because of ref module is not yet implemented its NAN 2.0 support.

TooTallNate · 2015-08-10T16:47:56Z

src/win32-dlfcn.cc

+
+#else
+
+    /* "Ken Thompson has an automobile which he helped design..." */


Why is this part necessary out of curiosity?

Depends how you define "necessary".

I think dlerror() is an extremely bad design, but it is a POSIX standard. The question becomes how hard to you want to work to implement it on Windows.

I'd be happy with my original implementation (always returning "?"), because IMHO, nobody should be relying on dlerror() to begin with.

The current implementation provides standard-compliant dlerror() functionality by returning a string with the Win32 error code (which can be looked up in WinError.h if ever needed).

What I didn't want to do is return the actual Win32 error text. Win32 error messages can be verbose. I'd either have to allocate a buffer that nobody will free (see also: memory leak), or create a large static buffer. For what? A message nobody should be relying on.

Well the basic implementation above seems adequate to me, and with the #if above this is dead code. Maybe remove to avoid future confusion?

TooTallNate · 2015-08-10T16:49:31Z

I'm 👍 for this, but ya we'll have to make sure all the tests are in order. It could be technically considered a breaking change for Windows users who might have been depending on the previous null behavior, but I'm contemplating whether we can consider it a bug fix instead since the behavior was incorrect from the get-go.

Created ExportedFunction() with C naming and calling conventions, which dynamic_library.js can use to to test symbol lookup. The function is never actually called.

Also added some additional comments about Windows, with note on how the test case could become invalid in the future.

unbornchikken · 2015-08-10T18:59:01Z

Cool, it's finally ready. I try to make a NAN 2.0 / io.js 3.0 support PR to ref module on this week, and travis is gonna be happy again.

(But I'll miss the reference to Ken Thompson's automobile :-)).

unbornchikken · 2015-08-14T20:40:09Z

I'll merge this once NAN 2.0 PR gets ready (after this gets landed: TooTallNate/ref#33), and we can release those together with all tests passed on Travis.

TooTallNate · 2015-08-31T23:41:56Z

I have this merged in the v2 branch. v0.10 seems to be failing one test. See https://ci.appveyor.com/project/TooTallNate/node-ffi/build/80. @mcnameej Any ideas there?

[00:01:36]   1) Library should accept `null` as a first argument:
[00:01:36]      Error: Dynamic Symbol Retrieval Error: Win32 error 127
[00:01:36]       at DynamicLibrary.get (C:\projects\node-ffi\lib\dynamic_library.js:112:11)
[00:01:36]       at C:\projects\node-ffi\lib\library.js:50:19
[00:01:36]       at Array.forEach (native)
[00:01:36]       at new Library (C:\projects\node-ffi\lib\library.js:47:28)
[00:01:36]       at Context.<anonymous> (C:\projects\node-ffi\test\library.js:33:22)
[00:01:36]       at callFn (C:\projects\node-ffi\node_modules\mocha\lib\runnable.js:289:21)
[00:01:36]       at Test.Runnable.run (C:\projects\node-ffi\node_modules\mocha\lib\runnable.js:282:7)
[00:01:36]       at Runner.runTest (C:\projects\node-ffi\node_modules\mocha\lib\runner.js:419:10)
[00:01:36]       at C:\projects\node-ffi\node_modules\mocha\lib\runner.js:526:12
[00:01:36]       at next (C:\projects\node-ffi\node_modules\mocha\lib\runner.js:340:14)
[00:01:36]       at C:\projects\node-ffi\node_modules\mocha\lib\runner.js:350:7
[00:01:36]       at next (C:\projects\node-ffi\node_modules\mocha\lib\runner.js:283:14)
[00:01:36]       at Object._onImmediate (C:\projects\node-ffi\nod
[00:01:36] e_modules\mocha\lib\runner.js:318:5)

mcnameej · 2015-09-01T04:31:39Z

@TooTallNate: The problem was that older versions of node.exe don't export a node_module_register function. I changed the test to use uv_fs_open instead. PR #232 contains the fix for v2 branch.

TooTallNate · 2015-09-04T20:19:21Z

Merged. Thanks @mcnameej!

mcnameej added 2 commits August 7, 2015 18:09

Removed dlfcn-win32.

a3c397c

Added simple-dlfcn-win32.

06c8323

(1) Move Win32 dlfcn code from deps to src.

cb8ccd4

(2) Convert filename from UTF-8 to UCS-2 and use LibraryLibraryW(). (3) Code clean up and hardening.

Updated test cases.

d4239d1

(1) Avoid depending on POSIX dynamic library semantics. (2) Check handling of Unicode characters in path/filename.

unbornchikken mentioned this pull request Aug 10, 2015

New dlfcn-win32 PR has landed and integrated there (in progress) #225

Closed

TooTallNate reviewed Aug 10, 2015
View reviewed changes

mcnameej added 2 commits August 10, 2015 12:46

Provide an explicit exported symbol for testing.

0fde429

Created ExportedFunction() with C naming and calling conventions, which dynamic_library.js can use to to test symbol lookup. The function is never actually called.

Use different "null as a first argument" tests on Windows and POSIX.

0eaf03f

Also added some additional comments about Windows, with note on how the test case could become invalid in the future.

mcnameej mentioned this pull request Aug 10, 2015

Added note about bundled dependencies have different terms (e.g. LGPL). #227

Closed

Dead code elimination.

96ee812

(But I'll miss the reference to Ken Thompson's automobile :-)).

unbornchikken mentioned this pull request Aug 29, 2015

io.js 3 support (done) #231

Closed

Merge remote-tracking branch 'upstream/master' into simple-dlfcn

89d39a2

mcnameej mentioned this pull request Sep 1, 2015

Fix "null as a first argument" test on Windows #232

Merged

TooTallNate closed this in f954bb7 Sep 4, 2015

mcnameej deleted the simple-dlfcn branch September 5, 2015 18:10

mcnameej mentioned this pull request Sep 5, 2015

Dll pathname with non-ascii characters #183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace dlfcn-win32 with simple-dlfcn-win32 #226

Replace dlfcn-win32 with simple-dlfcn-win32 #226

mcnameej commented Aug 7, 2015

unbornchikken commented Aug 8, 2015

mcnameej commented Aug 8, 2015

unbornchikken commented Aug 8, 2015

mcnameej commented Aug 8, 2015

unbornchikken commented Aug 10, 2015

unbornchikken commented Aug 10, 2015

mcnameej commented Aug 10, 2015

unbornchikken commented Aug 10, 2015

TooTallNate Aug 10, 2015

mcnameej Aug 10, 2015

TooTallNate Aug 10, 2015

TooTallNate commented Aug 10, 2015

unbornchikken commented Aug 10, 2015

unbornchikken commented Aug 14, 2015

TooTallNate commented Aug 31, 2015

mcnameej commented Sep 1, 2015

TooTallNate commented Sep 4, 2015


		#else

		/* "Ken Thompson has an automobile which he helped design..." */

Replace dlfcn-win32 with simple-dlfcn-win32 #226

Replace dlfcn-win32 with simple-dlfcn-win32 #226

Conversation

mcnameej commented Aug 7, 2015

unbornchikken commented Aug 8, 2015

mcnameej commented Aug 8, 2015

unbornchikken commented Aug 8, 2015

mcnameej commented Aug 8, 2015

unbornchikken commented Aug 10, 2015

unbornchikken commented Aug 10, 2015

mcnameej commented Aug 10, 2015

unbornchikken commented Aug 10, 2015

TooTallNate Aug 10, 2015

Choose a reason for hiding this comment

mcnameej Aug 10, 2015

Choose a reason for hiding this comment

TooTallNate Aug 10, 2015

Choose a reason for hiding this comment

TooTallNate commented Aug 10, 2015

unbornchikken commented Aug 10, 2015

unbornchikken commented Aug 14, 2015

TooTallNate commented Aug 31, 2015

mcnameej commented Sep 1, 2015

TooTallNate commented Sep 4, 2015