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
Merged

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Dec 4, 2020

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.

@TrueBrain TrueBrain force-pushed the TrueBrain:fix_news_crash branch from 01682da to 36fb6b7 Dec 4, 2020
@LordAro
Copy link
Member

@LordAro 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 TrueBrain force-pushed the TrueBrain:fix_news_crash branch from 36fb6b7 to d2eda62 Dec 5, 2020
@TrueBrain
Copy link
Member Author

@TrueBrain 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 TrueBrain force-pushed the TrueBrain:fix_news_crash branch from d2eda62 to 3d0f337 Dec 5, 2020
@TrueBrain
Copy link
Member Author

@TrueBrain 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).

TrueBrain added 2 commits Dec 4, 2020
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 TrueBrain force-pushed the TrueBrain:fix_news_crash branch from 3d0f337 to b05b316 Dec 5, 2020
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Dec 5, 2020

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.

@michicc
michicc approved these changes Dec 6, 2020
@LordAro
LordAro approved these changes Dec 6, 2020
Copy link
Member

@LordAro LordAro left a comment

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
6 checks passed
6 checks passed
Linux (clang)
Details
Commit checker
Details
Linux (gcc)
Details
Windows (x86)
Details
Windows (x64)
Details
Mac OS Mac OS
Details
@TrueBrain TrueBrain deleted the TrueBrain:fix_news_crash branch Dec 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants