Heap corruption when using Get/SetUpValue about keralua HOT 13 CLOSED

I just updated my main project to 1.0.11 and can confirm that it works. Thanks for helping me get this resolved quickly!

from keralua.

BTW, in case you're wondering what I'm trying to do, I'm trying to sandbox code by modifying it's environment. Upvalue 1 on a function is supposed to be the _ENV variable.

See eg: https://stackoverflow.com/questions/14290527/recreating-setfenv-in-lua-5-2

from keralua.

Also, one clarification:

my application terminates when I use GetUpValue or SetUpValue.

What I mean is, when the crash happens when they would be expected to work. If they would normally return an error, they do not crash, and return null as expected.

from keralua.

I am not sure what you trying to do, but GetUpValue usually is used when you push a C closure. which is not the case here.

if you make your program using C api with only C, it works ?

from keralua.

According to the people in the thread I posted (and other threads), it should work with lua functions. And according to the official documentation, it should work with lua functions.

But, even if you don't understand the use case, I think we can both agree that it shouldn't corrupt the heap and crash.

edit: I don't know if it happens in C. I tried making a C project, but I don't know C and couldn't get it to compile properly. This is why I use .Net

from keralua.

Hi @pkmnfrk thank you for your report.

Well I agree it shouldn't 😕. But KeraLua only p/Invokes into the Lua library. If GetUpValue is crashing then Lua is crashing. This is why you should repro this using the C library, and ask help in the Lua mailing list: https://www.lua.org/lua-l.html
If works with the C library but not KeraLua then is a bug on KeraLua I will be happy to fix it.

from keralua.

So, I have no idea how valid of a test this is, but I figured out how to make the C version work (it involved not making a VS project). I made a repo with both a C and a C# program that are as close as I can feasibly make them. The C# program crashes, while the C program does not.

https://github.com/pkmnfrk/luabug

C version outputs:

Stack: 1
bar

C# version outputs:

Stack: 1

... and then crashes.

I realize that KeraLua is a very thin binding on top of Lua proper, and there isn't much that could go wrong. I haven't yet ruled out that I am making some mistake here, and I just don't know it.

from keralua.

I have a bit more information, which may or may not help, but at least might point us in a better direction.

When running the C# project in mono on my Macbook, I get a more useful runtime error from the crash (although it still crashes at the same point):

https://gist.github.com/pkmnfrk/2ccb3f3ea99b75b7b78ddad00a2a12e3

At this point, it's clear that something weird is happening. Do you set a custom memory allocator for Lua, or rely on the default one in the aux lib? If so, it seems like there might be a latent bug lurking there. If not, then that suggests that memory is getting super corrupted somewhere...

from keralua.

I started investigating the possibility of the allocator being at fault, and I do not believe it is related. I updated the project to add a custom allocator (the malloc branch), and got this output:

https://gist.github.com/pkmnfrk/603f81e2b225f7dbbb7cbcaf0e3abb35

(It logs allocations as they happen, and keeps track of them so it can dump them out right before the crash)

The pointer that is being freed is not one that was allocated by the allocator, nor does it seem to ever be within a reasonable range of one that was (though, it's not so far out that it doesn't resemble one!). It also isn't being freed by the custom allocator, which is weird, because I thought lua didn't do any allocations by itself.

I think there are two possibilities:

1. There is a latent bug in lua being triggered by ??? in KeraLua
2. There is some kind of marshalling error. Though, this theory is suspect because the crash is clearly happening inside the p/invoke

At this point, I'm more confused than when I started. I'm not entirely sure where to go next from here...

from keralua.

It seems the crash is in the Marsheler.
Can you check in your C program what is the return value of lua_setupvalue and if is a valid string, could be a bug on Lua where it return a invalid char * doesn't crash on C because your not reading, but the CLR/Mono marsheler is trying to read. If this is the case maybe we need to change the return Type to IntPtr

from keralua.

Hm, well, I did as you asked:

const char* upValueName = lua_setupvalue(lua, -2, 1);
    
    if(upValueName)
    {
        printf("Successfully set upvalue %s (0x%08x)!\n", upValueName, (int)upValueName);
    }
    else
    {
        printf("Uh oh, upValueName is null........ this is about to go bad\n");
    }

However, the return value appears to be valid:

Stack: 1
Successfully set upvalue _ENV (0xb2402e68)!
bar

Let me quickly try adding a custom P/Invoke myself with an IntPtr return, and see if you're on the right track

from keralua.

So. Um, not sure what to make of this. I added this p/invoke:

[DllImport("lua53", CallingConvention = CallingConvention.Cdecl, CharSet = CharSet.Ansi)]
internal static extern IntPtr lua_setupvalue(IntPtr luaState, int funcIndex, int n);

And modified the body:

            //lua.SetUpValue(-2, 1);
            var ret = lua_setupvalue(lua.Handle, -2, 1);

            if(ret != IntPtr.Zero)
            {
                Console.WriteLine("Got a valid pointer back! {0:x}", (int)ret);
                Console.WriteLine("About to try and marshall back into a string... wish me luck!");

                string res = Marshal.PtrToStringAuto(ret);

                Console.WriteLine("Success! {0}", res);
            }
            else
            {
                Console.WriteLine("Uh oh, ret is null...... this is about to be bad");
            }

And... well:

Got a valid pointer back! 78f18f70
About to try and marshall back into a string... wish me luck!
Success! _ENV
bar

So, uh, I don't know why it works, but letting the p/invoke do the work fails. It seems like they are equivalent, but clearly they are not.

(Note that this result is on my mac with Mono, I will test the work around on Windows when I get home in a few hours)

from keralua.

Back on my Windows PC, I tested it and got identical results (save for the pointer value). And, just to confirm the negative case, when I switch the extern back to return a string instead of IntPtr, I get the crash again.

This thread seems relevant: https://stackoverflow.com/questions/52762207/net-4-0-char-to-string-interop-crashes-on-x64-0xc0000374

I think I see what's going on here, kinda. Basically, this crash is happening specifically because I'm trying to change _ENV. I bet that if I modified any other upvalue, it would work.

Specifically, I suspect the string "_ENV" is baked into the LUA binary. Unlike all the other strings, this one is not dynamically allocated. Thus, when .NET tries to free it, it fails. Unfortunately, it seems there isn't any way for us to tell the CLR not to take ownership of the string. Thus, your instinct of replacing the argument with IntPtr was correct.

(This does raise the question of other strings, like the ToString(int) method. If the CLR is freeing these strings too, that means it's not going through the LUA allocator, which is troubling.)

I think the proper solution to this problem, then, is two-fold:

1. Change the lua_getupvalue and lua_setupvalue externs to return IntPtr
2. Modify Lua.GetUpValue and Lua.SetUpValue to manually Marshal the string.

That said, I must admit that I don't fully understand the semantics behind all this native/CLR interop. If anything I just said doesn't make sense, then please correct me. Otherwise, I will prepare a pull request post-haste!

from keralua.