New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server crashes in function call ClickActionToString(355) #4270

Open
Seadragon91 opened this Issue Jul 29, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@Seadragon91
Copy link
Contributor

Seadragon91 commented Jul 29, 2018

Since pull request #4161 this function calls will crash the server;
ClickActionToString(355)
cBoat:MaterialToItem(355)
cBoat:MaterialToString(355)

Is the best fix to just return default values for the functions that are accessible from lua code?

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Jul 29, 2018

I think the best solution is to fix tolua++ so that it is correctly reported as an error. I've noticed that there is a function tolua_iseMaterial but the bindings don't actually use it. Maybe that's a good place to start.

@Seadragon91

This comment has been minimized.

Copy link
Contributor

Seadragon91 commented Jul 29, 2018

The function ClickActionToString can be fixed by changing int a_ClickAction to eClickAction a_ClickAction. Not sure why the param is a int.

inline const char * ClickActionToString(int a_ClickAction)

Fixing the other problem in tolua++ is a good idea. My current analysis: The enum eMaterial belongs to the class cBoat and in tolua++there is (maybe) missing a ref to the class and it can not find cBoat::eMaterial, as it looks only for eMaterial.

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Jul 29, 2018

That's not the right solution.
The point of that function is to convert a number that may represent a ClickAction into a string. If it is not a valid ClickAction, it should report an error and bail out.

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Jul 29, 2018

I'm not sure I understand where you're coming from @madmaxoft.
I've checked and every caller apart from lua already knows it has a valid eClickAction. Also, prior to #4161 there was already an assert in ClickActionToString that would have shutdown the server anyway. It seems to me that you're suggesting weakening the type system on the C++ side just to work around a bug in the tooling.

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 2, 2018

My fault. This function has a bad signature indeed. The thing I was referring to was a way to convert generic numbers to eClickAction, that can throw errors if an unknown number is passed into it.

@Seadragon91

This comment has been minimized.

Copy link
Contributor

Seadragon91 commented Aug 3, 2018

Ah yes it will still crash if the number is between 30-254 as tolua++ creates a range check and here the click action ranges is 0-29 and then comes number 255.

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 3, 2018

We need a proper IntToClickAction function that translates an int into eClickAction or raises an error on invalid input, and use that in all bindings to check the enum values passed from Lua. And do that for all enums.

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 3, 2018

However, I'd postpone this until we decide whether we do want to move to sol2 or not (#4276) since that could change things around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment