Add null pointer protection

[adamlwgriffiths]

Most functions dont check if a window/monitor/etc pointer is null. Add checks and throw exceptions.

[Emilgardis]

That went quickly, I just pressed the button and it closed. Oh well. I don't think there is a reason to reopen right now. In the examples we even check ourselves if for example CreateWindow returns Null (None)

[adamlwgriffiths]

Don't think this is resolved. It's not so much the output, as the input. There are many functions that don't check incoming objects for validity

Eg:

def GetMonitorPos(Monitor monitor):
    cdef int x, y
    cglfw3.glfwGetMonitorPos(<cglfw3.GLFWmonitor*>monitor._this_ptr, &x, &y)
    return x, y

[Emilgardis]

But wouldn't adding this kind of security slowdown the code? Is it even needed?

[adamlwgriffiths]

Null/Invalid pointer dereference is an insidious, and often subtle, programming error. It doesn't always lead to a crash, and when the C runtime detects a memory out of bounds error will simply give you a horrible stack trace. These are hard to track down in Python code. It's much better to detect bad objects in Python and provide proper warnings before we hit the C level, where we can trigger random memory over-writes.

Any check would be quite trivial, a simple if a is None would probably suffice in most cases. Better even to do an isinstanceof check too to ensure it's the GLFW object is the right one (don't want to send a window pointer as a monitor pointer, etc). It's the equivalent of a null check and something like a vtable lookup, something C/C++ would do a lot anyway. Not to mention, GLFW doesn't get called much. I'd rather spend the extra time I get from error checking optimising code, than having to hunt subtle bugs =P

[Emilgardis]

Ok, that would be easily implemented I guess. I suspect this is needed for every get method and not for every set?

[adamlwgriffiths]

I was intending to cover the set functions, but thinking about it, all of them need to be covered. Anywhere an object is converted to a pointer from Python, or a pointer is converted to an object from GLFW, we need to do a check.

I'm not sure if Cython does any inbuilt pointer checking and error handling, but it would be better to instead check ourselves and throw something like ValueError('Monitor parameter was invalid')

For example:

def GetMonitorPos(Monitor monitor):
    cdef int x, y
    cglfw3.glfwGetMonitorPos(<cglfw3.GLFWmonitor*>monitor._this_ptr, &x, &y)
    return x, y

Would become something like:

def GetMonitorPos(Monitor monitor):
    cdef int x, y
    if not monitor:
        raise ValueError('monitor parameter must be set')
    cglfw3.glfwGetMonitorPos(<cglfw3.GLFWmonitor*>monitor._this_ptr, &x, &y)
    return x, y

Because that function is typed (Monitor monitor), it shouldn't be possible to send through a different object type, so I don't think an isinstance check is required.

As for output, what you say is true because we wrap pointers/C++ object with Python objects.

Ie:

def GetPrimaryMonitor():
    cdef const cglfw3.GLFWmonitor* c_monitor = cglfw3.glfwGetPrimaryMonitor()
    monitor = Monitor()
    monitor._this_ptr = c_monitor
    return monitor

If cglfw3.glfwGetPrimaryMonitor() returns null, the user won't know and will try and use the Monitor object because it's wrapped in Python. Ie, that function doesn't return None if the result of cglfw3.glfwGetPrimaryMonitor() is null.

It really should be something like this:

def GetPrimaryMonitor():
    cdef const cglfw3.GLFWmonitor* c_monitor = cglfw3.glfwGetPrimaryMonitor()
    if c_monitor != null:
        monitor = Monitor()
        monitor._this_ptr = c_monitor
        return monitor
    return None

That way if the call fails, you get None as expected.

[Emilgardis]

Couldn't we add a set property for _this_ptr that checks if it gets a null object?

[adamlwgriffiths]

It would still return a Monitor python object which resolves to True in a Boolean check.

[adamlwgriffiths]

Although some functions should probably raise exceptions on failure. Eg.

window = createWindow(a,b,c)
monitor = getPrimaryMonitor()

I would expect these to either succeed, or catastrophically fail with an exception.

[filonik]

Regarding True in a boolean check: What's wrong with having a nonzero method like I put in the Image class?

def __nonzero__(self):
    return self._this_ptr != NULL

Exceptions would probably be a more pythonic solution, but adding nonzero would be quick an easy.

[Emilgardis]

I agree that it would be quick and easy. If you look at PyOpenGL, they always check for an error on every function call. This slows down code! GLFW functions are not called often, and this means that checking for errors could be profitable. What I propose is that we check for errors where errors can occur outside the users code.

[adamlwgriffiths]

Adding that can't hurt. We can then see if the API works as expected. type(result) would still produce an object type. But no worse than an empty set, list, or string.

[filonik]

Well, to be precise, PyOpenGL's error checking is optional (at the expense of a slightly more complicated import mechanism).

import OpenGL
OpenGL.ERROR_CHECKING = False
from OpenGL.GL import *
from OpenGL.GLU import *

I agree that most GLFW functions are only called very infrequently, so adding error checking should not have a significant impact on performance. It just requires some implementation effort to come up with a consistent error reporting scheme and define the respective exceptions. This adds to the complexity of the wrapper code, which right now is very thin.