Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: fix nullptr deletion in DeleteWindowById #8941

Merged
merged 1 commit into from Apr 10, 2021
Merged

Conversation

@perezdidac
Copy link
Contributor

@perezdidac perezdidac commented Apr 4, 2021

Motivation / Problem

DeleteWindowById in window.cpp has incorrect logic since delete w is executed if w == nullptr. The delete operator is overloadad but it seems to do nothing, so I assume that got mistakenly added at some point. I've observed this debugging and when the game is idle it gets continuously called by the news loop trying to close the latest window.

Description

Fixing code in DeleteWindowById that can call delete w where w is nullptr.

Limitations

Tested by playing the game a little bit, and all works fine.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
@perezdidac perezdidac marked this pull request as ready for review Apr 4, 2021
@LordAro
Copy link
Member

@LordAro LordAro commented Apr 4, 2021

afaik, calling delete on nullptr is explicitly defined to be "ok", though is definitely a bit weird. Can you elaborate on the issue this is solving? https://en.cppreference.com/w/cpp/language/delete

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 4, 2021

afaik, calling delete on nullptr is explicitly defined to be "ok", though is definitely a bit weird. Can you elaborate on the issue this is solving? https://en.cppreference.com/w/cpp/language/delete

I am trying to remove the w == nullptr from the condition, given that it does makes zero sense. The condition basically lets the delete w operation to happen if w == nullptr , intentionally! Which it has to be a mistake from a previous code change, since I can't think of a reason for wanting that to happen. I've been reading about it too and yeah it seems that delete 0 is fine and that's not going to change, but doing it intentionally is still technically incorrect and it's using CPU cycles to call the delete operation from the runtime the process is running with.

Copy link
Contributor

@nielsmh nielsmh left a comment

I'm in the "this is pointless, leave it as-is" camp, since yes delete nullptr is legal and defined to be a no-op. Whether this actually saves CPU cycles is difficult to answer, but any savings are unlikely to be significant.
Regardless, you need to address the compiler warnings produced at least.

@nielsmh nielsmh dismissed their stale review Apr 4, 2021

Warnings fixed (but the change is still pointless)

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 4, 2021

I'm in the "this is pointless, leave it as-is" camp, since yes delete nullptr is legal and defined to be a no-op. Whether this actually saves CPU cycles is difficult to answer, but any savings are unlikely to be significant.
Regardless, you need to address the compiler warnings produced at least.

Applied the changes to address the compiler warnings.

I don't disagree with fact that delete nullptr is and will be legal. The reason this code changes still makes sense to me is because the existing code is intentionally checking if w is null to then call delete.

If there was a case where a given pointer gets initialized to null and somewhere in the code we call delete for that pointer, I would be fine with that, but again in this case it's the explicit check for w being nullptr what makes it bad.

@glx22
Copy link
Contributor

@glx22 glx22 commented Apr 4, 2021

Existing code checks intentionally for nullptr not for the delete, but for w->flags.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Apr 4, 2021

Hihi: just because something is "technically okay" doesn't mean it makes sense to read as developer :P

What triggered me more, and what I wonder about: why is the last news window being deleted so many times? Was that a local debug, or is master doing that? That sounds a bit fishy too :D

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 4, 2021

Existing code checks intentionally for nullptr not for the delete, but for w->flags.

Mmm. So if w is null just delete, but if it's not then check the flag before delete? Still smelly to me.

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 4, 2021

Hihi: just because something is "technically okay" doesn't mean it makes sense to read as developer :P

What triggered me more, and what I wonder about: why is the last news window being deleted so many times? Was that a local debug, or is master doing that? That sounds a bit fishy too :D

Yes absolutely. It was a dev build but I had the feeling it should happen in a release build too. Lemme take a closet look at how often and why that happens, since I saw it happening but didn't see newspaper showing up.

@Milek7
Copy link
Contributor

@Milek7 Milek7 commented Apr 4, 2021

Existing code checks intentionally for nullptr not for the delete, but for w->flags.

Mmm. So if w is null just delete, but if it's not then check the flag before delete? Still smelly to me.

In current code w == nullptr prevents evaluation of w->flags in case w is nullptr. And deleting nullptr is legal. Sneaky, but correct.

@glx22
Copy link
Contributor

@glx22 glx22 commented Apr 4, 2021

It's a simplification of force || (w != nullptr && (w->flags & WF_STICKY) == 0)

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 5, 2021

I still see the value of changing this since as a developer I read this and it does not make much sense.

LordAro
LordAro approved these changes Apr 6, 2021
@LordAro LordAro merged commit 0cb99c5 into OpenTTD:master Apr 10, 2021
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants