potion icon indicating copy to clipboard operation
potion copied to clipboard

More segmentation faults

Open robotii opened this issue 10 years ago • 7 comments

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.

robotii avatar Oct 19 '15 10:10 robotii

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

rurban avatar Oct 19 '15 10:10 rurban

I've fixed potion_any_cmp with the following commit - https://github.com/robotii/potion/commit/8833effbb33ba18a21c2b2830ab569698956c39a

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

robotii avatar Oct 27 '15 13:10 robotii

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

rurban avatar Oct 27 '15 14:10 rurban

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.

robotii avatar Oct 27 '15 14:10 robotii

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

rurban avatar Oct 27 '15 14:10 rurban

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 https://github.com/robotii/potion/commit/5a3f9f80c146e3db3a4401348611615361259313 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.

robotii avatar Oct 27 '15 15:10 robotii

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

rurban avatar Oct 27 '15 15:10 rurban