Skip to content
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

More segmentation faults #87

Open
robotii opened this issue Oct 19, 2015 · 7 comments
Open

More segmentation faults #87

robotii opened this issue Oct 19, 2015 · 7 comments
Assignees
Milestone

Comments

@robotii
Copy link
Contributor

robotii commented Oct 19, 2015

The following lines each result in a segfault

Function()()

This can be fixed by a check in potion_call that the closure and closure->method is not null and returning PN_NIL if either are null. You might also want to output a warning that the method was null.

Error()

This is caused by the REPL trying to print the error object by converting to string, where the message is set to null. The offending call to potion_str_ptr being in potion_error_string.

Compiled()

Again this is caused by the REPL trying to print out the value. The problem occurs in potion_proto_string about 9 lines in, on the pn_printf statement.

The last two errors only occur in the REPL, and don't cause a problem unless they are called or converted to string, when the lack of data in the structs becomes a problem.

Possibly for Error, it would be nice to have a constructor that took a string and returned a valid Error object, but I can't see any reason why the Compiled constructor should be exposed at all.

Lobby cmp 0

This is caused by an infinite loop in potion_any_cmp, where there should probably be a check for lobby and if so, directly return a value.

@rurban
Copy link
Member

rurban commented Oct 19, 2015

Thanks. Do you care for a patch, so your name will be in the git log?

@robotii
Copy link
Contributor Author

robotii commented Oct 27, 2015

I've fixed potion_any_cmp with the following commit - robotii@8833eff

I'm not sure if this is the right way to go or not.

@rurban
Copy link
Member

rurban commented Oct 27, 2015

No, you changed self cmp value to NIL cmp value.
Checking for Lobby is the right way.

@robotii
Copy link
Contributor Author

robotii commented Oct 27, 2015

That's fine - I can do the check for Lobby.

The problem is, that this is called for any user objects that don't explicitly implement cmp. This is because Object does not explicitly implement cmp, which means that it is inherited from Lobby.

C = class (): /value = 0.
C() cmp 0

needs to be defined as well.

We can't compare the pointers by default, as this could result in the comparison changing after a garbage collection (well, we could if you don't mind the value being essentially random).

I'm not saying the way I did it is the right way, just that we need to make the comparison work for more than just Lobby. Either that, or we need to override the method in Object, which amounts to the same thing.

rurban pushed a commit that referenced this issue Oct 27, 2015
See GH #87.
Fixes Function()() (empty closure or method), Error() (empty error message),
Lobby cmp 0 (compare with Lobby is always 1, unless with Lobby which is 0).

string of Compiled() (proto_string) not yet fixed
@rurban
Copy link
Member

rurban commented Oct 27, 2015

I see. A missing cmp method should rather error I believe.

@robotii
Copy link
Contributor Author

robotii commented Oct 27, 2015

The error with Error() can be fixed in internal.c by adding something like

  if (e->message == PN_NIL)
    return potion_str_format(P, "** unspecified error");

into potion_error_string

This has the advantage that we can create our own Error objects and check for them, rather than disallowing access to the Error object.

I also added a constructor for Error in robotii@5a3f9f8 although this is still a work in progress and I haven't tested it yet.

My plan is to change potion_object_new and potion_class to disallow creating anything other than Object or user-defined types, and to disallow sub-classing in the same way. That way we would have to explicitly allow creation of "special" objects by creating a constructor for them.

Let me know if I'm on the right path with this, as I don't want to waste time, if it's not the right solution.

@rurban
Copy link
Member

rurban commented Oct 27, 2015

Sounds good.
I already fixed all other segv's, just not Compiled string. I need my linux box for this.

@rurban rurban self-assigned this Nov 5, 2015
@rurban rurban added this to the 0.4 milestone Nov 5, 2015
robotii added a commit to robotii/potion that referenced this issue Nov 10, 2015
Remove the possibility for the user to create a "special" object that doesn't have a constructor.
Disable subclassing any of the "special" types built-in to the system.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants