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: do not add an offset to a nullptr #8351

Merged
merged 2 commits into from Dec 6, 2020

Conversation

TrueBrain
Copy link
Member

While at it, prevent a potential cases where an offset would be added to a nullptr (which would be horrible wrong for completely different reasons).

Tnx to milek7 for tracing the root cause.

@LordAro
Copy link
Member

LordAro commented Dec 4, 2020

Are there any additional changes (asserts, etc) that could be added to prevent this mistake from being made in future?

@TrueBrain
Copy link
Member Author

TrueBrain commented Dec 5, 2020

Went for a slightly different direction; turns out there were more places that are or could potentially feed a nullptr in as object, fully assuming the sld is global. So I no reduced it to an assert(), that should tell us if this is the case. From what I can tell, it never happens currently. If the assert() does hit, something is really wrong, as a non-global variable is read as a global variable; this really would be random behaviour at that point.

@TrueBrain
Copy link
Member Author

TrueBrain commented Dec 5, 2020

And something is wrong: when saving the date, SlGlobList() is called which calls SlObject() with a nullptr, which calls GetVariableAddress() with a nullptr .. and on a non-global, with the address of 0. This on its turn returns a nullptr .. and for some reason that works :P

#4  0x0000555555c45c2b in GetVariableAddress (object=0x0, sld=0x5555568e7340 <_date_desc+128>) at /home/patric/projects/OpenTTD/OpenTTD/src/saveload/saveload.h:877
#5  0x0000555555c489bf in SlObject (object=0x0, sld=0x5555568e7340 <_date_desc+128>) at /home/patric/projects/OpenTTD/OpenTTD/src/saveload/saveload.cpp:1622
#6  0x0000555555c48a01 in SlGlobList (sldg=0x5555568e72c0 <_date_desc>) at /home/patric/projects/OpenTTD/OpenTTD/src/saveload/saveload.cpp:1633
#7  0x0000555555c3b772 in SaveLoad_DATE () at /home/patric/projects/OpenTTD/OpenTTD/src/saveload/misc_sl.cpp:125

This is caused by SLE_CONDNULL, a method to indicate there are bytes there, but we don't care about them anymore (supporting old savegames).

This is, by specs, undefined behaviour. See
https://reviews.llvm.org/D67122

In cases where this is done, optimizations done by LLVM can
generate code that causes crashes.

GetVariableAddress() had two (legit) ways this could happen:
- For SaveLoad set to global
- For SaveLoad set to SLE_VAR_NULL, where sld->address is always
  a nullptr, and object could or could not be a nullptr.
@TrueBrain
Copy link
Member Author

This function is something. This patch changes the behaviour a bit: when a SaveLoad is SLE_VAR_NULL, it will now always return a nullptr. In the old behaviour it could also return object. This shouldn't matter at all, as any read/write to the SaveLoad is a noop in these cases.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything about this bit of the code is horrifying, but I'm happy you've tested it well enough :)

@TrueBrain TrueBrain merged commit 731af1f into OpenTTD:master Dec 6, 2020
@TrueBrain TrueBrain deleted the fix_news_crash branch December 6, 2020 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants