Code mistakes found with TscanCode #1617
Conversation
| float value = 0.f; | ||
| static float value = 0.f; | ||
| m_value = &value; | ||
| revert_value(); |
There was a problem hiding this comment.
According to https://www.geeksforgeeks.org/static-keyword-cpp/ :
Static variables in a Function: When a variable is declared as static, space for it gets allocated for the lifetime of the program. Even if the function is called multiple times, space for the static variable is allocated only once and the value of variable in the previous call gets carried through the next function call.
This variable should be unique - in fact, it shouldn't even exist, and should be forcibly included in the ctor. It's my responsibility to fix that! 😆
| int value = 0; | ||
| static int value = 0; | ||
| m_value = &value; | ||
| revert_value(); |
| else if ((m_physic.get_velocity_y() <= 0)) { | ||
| m_sprite->set_action(sa_prefix+"-jump"+sa_postfix); | ||
| } | ||
| else if ((m_physic.get_velocity_y() <= 0)) { | ||
| m_sprite->set_action(sa_prefix+"-skid"+sa_postfix); | ||
| } | ||
| } |
There was a problem hiding this comment.
The bodies are different - it shouldn't be deleted right away. We should see in which case(s) Tux must skid/jump and fix the conditions accordingly.
There was a problem hiding this comment.
The skidding action is also set at another place in the code and skidding worked in the game when I tried it.
| if (m_action->family_name != newaction->family_name) | ||
| if (!m_action || m_action->family_name != newaction->family_name) |
There was a problem hiding this comment.
Is it possible for m_action to be empty at this point in the code? Should we use a non-pointer property?
There was a problem hiding this comment.
In the constructor m_action is set to some action from the SpriteData reference m_data, and the draw method only works if m_action is not nullptr. I don't know if a Sprite which cannot be drawn can exist.
| for (auto& mask : m_masks) | ||
| return mask->get_mask(); | ||
| if (!m_masks.empty()) | ||
| return m_masks[0]->get_mask(); |
There was a problem hiding this comment.
(I'm responsible for that one - I'm currently revamping the autotiles and I had spotted that error and a few others:) But thanks anyways for the fix, I have no idea what I was thinking when I wrote that 😆)
|
Question - Why "involves: security"? Are there security-critical exploits in the bugs found? |
|
I think since SuperTux is a singleplayer game there are inherently almost no security problems. Exceptions are the add-on downloading, downloaded script execution (or similar things) and availability problems (e.g. crash on startup when a configuration is wrong; I'm not sure if this is safety or security). |
|
Can you please rebase? I'll merge it afterwards. |
|
@tobbi Don't merge it yet - I believe those static variables in the UI might cause a memory leak. I'd like to fix those myself. @HybridDog Do I have your permission to edit the two static variables? |
Yes, please do it. I think my changes in game_session.cpp may not make much sense. I don't know in which cases m_currentsector can be null. |
|
Forgot to write it yesterday, but it's ready now 👍 |
HybridDog
force-pushed
the
m_tscan_detecteds
branch
from
adaf175 to
ce5bef4
Compare
|
Thanks, @HybridDog |
I executed TscanCode on SuperTux' src folder and it found these errors: tscan_result.txt
I tried to fix them but I don't understand all code sections, so I don't know if all my changes make sense.