this can be NULL - segmentation fault

[yamgent]

I get a segmentation fault with the latest commits.

The assertion for (this != NULL) fails at Sources/Engine/Base/Anim.cpp:182. Ignoring this assertion, the code will proceed to crash.

lldb_log.txt

I looked at the commit that made the changes to this portion (5badefa). The compiler does not guard against function calls for NULL "objects" (the function executes anyway on inexistent objects). Therefore, even though the compiler is correct that 'this' pointer cannot be null in well-defined C++ code, the assumption that the code is well-defined in the first place is faulty.

I propose 2 different possible solutions:

1. Either revert commit 5badefa, or
2. Add a if statement to all affected method calls so that it checks that the object is not NULL first before calling its methods. I.e.

Before

// will call even if abc is NULL
abc->CallFunction();

After

// now no longer calls when abc is NULL
if (abc != NULL)
    abc->CallFunction();

The second solution is probably "proper" C++.

Note: I looked through the rest of the changes in the commit and some portions will require more work than adding one if statement. (E.g. CConsole::GetBufferSize() requires the value of 1 to be returned if this == NULL)

[DanielGibson]

oh cool, this happens directly at start - I thought @emlai had tested this.. but I didn't either, when rebasing his changes ontop of mine, I only made sure it compiles :-/

I still hope that at least most of those this != NULL checks were superfluous and we can get away with adding a few NULL-checks in the calling code, but if this turns out to bad we just indeed revert the commits, hoping optimizing compilers don't break it by optimizing the checks away..

I did that in #56, with those changes I can start and quit the game without crashes, and I can also start the first level and play a bit. I didn't test much more, though, so maybe there are other places where NULL-checks are needed. Could you try my fix? :-)

[yamgent]

Thanks for the fix!

Unfortunately I still cannot start the game as it now crashes on Sources/Engine/Base/Serial.cpp:137. The NULL assert at Sources/Engine/Sound/SoundData.cpp:263 was triggered before the crash.

I realized that if you look at these codes from the old C "functions" point of view (rather than from C++ "object oriented" point of view), all these checks actually make sense. They were probably added because they had a need for objects to be NULL sometimes.

We can still avoid having to revert by looking at commit 5badefa and adding all of the necessary checks to other cases not covered in the fix.

[DanielGibson]

are you sure you tested my fix from #56? the assert in SoundData.cpp:263 (CSoundData::RemReference()) really shouldn't happen anymore, as I check for NULL in the callers

(if you are sure, can I have a backtrace?)

[yamgent]

Oops, sorry, my bad, I applied the wrong branch. Sorry for the confusion.

Seems to be working fine now. :)

[DanielGibson]

Awesome, thanks for testing! :)