NULL pointer dereference fix

[ghost]

It occurs when we scroll while a cursor points to the terminal window.

[BatchDrake]

This was a known bug but I couldn't find time to fix it by myself, thank you! However -if you have time, although it's not critical- could you do the check in generic_drag_onmousedown (map.c) instead?

The actual bug here is that every time I scroll with the mouse wheel on a widget I assume there is a struct filemap * in its opaque data, which is essentially false. We'll just have to check simtk_widget_get_opaque returns something different from NULL under the cases SIMTK_MOUSE_BUTTON_UP and SIMTK_MOUSE_BUTTON_DOWN.

I just want to assume that calling filemap_scroll with a NULL filemap doesn't make sense, so next time there's an improper use of this function it will crash again and we'll get to the root cause easily instead of ignoring it blissfully.

Can you do it? If you don't have time for this, just tell me. I can do the merge now and fix it properly whenever I can.

[ghost]

Are unexpected crashes a better way to uncover errors? Is it a caller job to perform checks like this? I think it's a good choice to return value from filemap_scroll(), so we can handle errors properly in caller function without a segfaults, aren't you?

[BatchDrake]

Well, I think that if we call filemap_scroll against a filemap is because we have a filemap to begin with. I'm understanding it like a regular object in the sense of OOP: when you call a method against a null instance, you should expect an exception (if not a crash). The key question here is, does it make sense to scroll something that just doesn't exist?

I would agree with you if dealing with null pointers could have sense in some other context, for instance, if we had a function that took a filemap as an optional parameter. Or if it was an API function we exported to programmers to code extensions, so it could ease debugging their programs. But for the time being, it's internal, although this may change in the future and it could be reconsidered (along with changing the behavior of many other functions).

I'll give you another reason why I belive it's a good idea: let's say that filemap_scroll ignores null pointers silently. The application has been assuming from the beginning that some widget has been holding a valid filemap information. Then that filemap pointer (which is null) is copied into another variable for whatever reason (assuming, as I previously said, that is not null), and then that variable is put into a structure and so on, with no checks at all. Then, when the application crashes somewhere else because it tried to use that filemap, we have to go back through all copy steps to figure out what went wrong in the first place.

Of course we can check for a null pointer in every single copy step, but we could save many lines of code if we put a single check before we start assuming that we have a valid pointer.

What do you think?

2015-02-22 19:47 GMT+01:00 Sergey Zelenyuk notifications@github.com:

Are unexpected crashes a better way to uncover errors? Is it a callee job to perform checks like this? I think it's a good choice to return value from filemap_scroll(), so we can handle errors properly in caller function without a segfaults, aren't you?

— Reply to this email directly or view it on GitHub https://github.com/BatchDrake/vix/pull/6#issuecomment-75450541.

Gonzalo José Carracedo Carballal

[ghost]

I'm understanding it like a regular object in the sense of OOP: when you call a method against a null instance, you should expect an exception (if not a crash). The key question here is, does it make sense to scroll something that just doesn't exist?

Yes, it doesn't, but then I see some new problems: 1) we should explicitly determine what arguments are objects and what aren't, and handle they differently; 2) we should trust to any internal function what the next call will not change a pointer to the object (i.e., by realloc) and it's still valid; 3) it's not so obvious for a new code readers like me, who think of it as an oversight, and it isn't.

But for the time being, it's internal, although this may change in the future and it could be reconsidered (along with changing the behavior of many other functions).

I agree, so let's fix current problem as you want. I shall update this pull request soon.

[BatchDrake]

Yes, you are right. I've just written a brief summary about how to use OOP in ViX and how it is implemented: https://github.com/BatchDrake/vix/wiki/Coding-style,-standards-and-conventions

This is just a draft and needs to be completed. Please tell me whenever you find something I should explain about ViX code.

2015-02-22 22:21 GMT+01:00 Sergey Zelenyuk notifications@github.com:

I'm understanding it like a regular object in the sense of OOP: when you call a method against a null instance, you should expect an exception (if not a crash). The key question here is, does it make sense to scroll something that just doesn't exist?

Yes, it doesn't, but then I see some new problems: 1) we should explicitly determine what arguments are objects and what aren't, and handle they differently; 2) we should trust to any internal function what the next call will not change a pointer to the object (i.e., by realloc) and it's still valid; 3) it's not so obvious for a new code readers like me, who think of it as an oversight, and it isn't.

But for the time being, it's internal, although this may change in the future and it could be reconsidered (along with changing the behavior of many other functions).

I agree, so let's fix current problem as you want. I shall update this pull request soon.

— Reply to this email directly or view it on GitHub https://github.com/BatchDrake/vix/pull/6#issuecomment-75460758.

Gonzalo José Carracedo Carballal

[ghost]

Sorry, I have created a new pull request (#7) because of re-cloning of my fork of vix.