search

WohlSoft / LuaJIT

Fork of LuaJIT repository which adds CMake build and a small change that forces usage of UTF8 (instead of ANSI) in file system paths on Windows
http://luajit.org
Other
40 stars 16 forks source link

Prevent crash during ffi.load() when dlerror() returns NULL #13

Closed mcclure closed 5 years ago

mcclure commented 5 years ago

The ffi.load() implementation assumes the string returned from dlerror() is non-NULL and immediately dereferences it. Per the standard definition of dlerror, this should always be safe, but we have observed dlerror() returning NULL on Android (Oculus Quest) anyway, which causes a crash. Patch issues a well-formed error instead of crashing.

I have submitted this in parallel to the normal luajit repo https://github.com/LuaJIT/LuaJIT/pull/522 but I'm not actually sure what the patch process is for luajit.

Wohlstand commented 5 years ago

Anyway, I think, I'll add this change myself, so, thank you for a workaround :fox_face:

mcclure commented 5 years ago

Thanks! If you want to make note of it, the Android (Quest) device that returned the NULL gave me

$ adb shell getprop ro.build.version.release
7.1.1
$ adb shell getprop ro.build.version.sdk
25

I don't know if this is a universal behavior for Android 7/Nougat or if it is unique to this device.

Wohlstand commented 5 years ago

Anyway, I will just add this as a note to understand for what platform this workaround targeted and where it was originally recognized. So, if it will happen again on someone's device, then, they are will see the source of a trouble as a soft error rather a destructive and painful crash :fox_face:

mcclure commented 5 years ago

That makes sense! By the way, I think your most recent commit contains an error. It looks like you pasted the getprop stuff outside of the comment instead of inside of it. I don't think that will compile.

In my opinion it is enough to say that it was seen in Android 7.1.1 SDK 25. The getprop stuff is probably unnecessary to include.

Thanks

Wohlstand commented 5 years ago

It looks like you pasted the getprop stuff outside of the comment instead of inside of it. I don't think that will compile.

Yeah, I have found this immediately, and it's just a stupid :man_facepalming: I have fixed it with a next commit. So, I'll better to squish this and force-push here then.

Wohlstand commented 5 years ago

Done, so, now it should be correct :ok_hand: :fox_face:

mcclure commented 5 years ago

@Wohlstand , I cherrypicked your commit into the Moonjit version of my PR, I understand this to be in line with the license on WohlSoft/LuaJIT but I just wanted to let you know

Wohlstand commented 5 years ago

@mcclure, My repo is just a copy of the original with minor tweaks like enforcing UTF8 in file paths even on Windows (to avoid ANSI bullshit) and CMake build support I needed to don't deal with GNU Make limitations and build LuaJIT easily for everything. In all other cases, LuaJIT is matching to the mainstream. Yeah, the license is absolutely the same as the mainstream. Anyway, you could do that without any notice as the license is permissive, but thanks for a note and for one another "hello" :fox_face: :wink:

Wohlstand commented 5 years ago

Oh, @mcclure , I have reviewed a bit the MoonJit project, and I am interested, what was changed in comparison to the original? I think you may also pick-up my UTF8 paths support (it replaces fopen and freopen with macro which is same everywhere except of Windows where are proxy functions are converting UTF8 into UTF16 for WinAPI _wfopen and _wfreopen, it can be enabled via macro, otherwise, on Windows, it will have default behavior with ANSI crap) and CMake build that simplifies dealing with different platforms :fox_face: :wink:

mcclure commented 5 years ago

@Wohlstand, MoonJIT is a fork of LuaJIT with many bug fixes not in trunk. A developer I work with says MoonJIT is likely to be "the most up to date LuaJIT" for the forseeable future, and that in their project it resulted in significant RAM gains.

I am with https://lovr.org (https://github.com/bjornbytes/lovr). We currently use WohlSoft LuaJIT because we require CMake for Android support. Neither LuaJIT trunk nor MoonScript has CMake.

I think it would be interesting to merge MoonScript together with the Wohlsoft changes. One way to do this would be if you created PRs for your Unicode and CMakeList changes and submitted them to MoonScript. Another possibility is I could create a Lovr organization LuaJIT which merges together Wohlsoft master with Moonscript master.