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 #8713: Change OTTD2FS and FS2OTTD to return string objects #8716

Merged
merged 2 commits into from Apr 7, 2021

Conversation

@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Feb 21, 2021

Motivation / Problem

The OTTD2FS and FS2OTTD functions are not thread safe, they use static (global) buffers to hold the returned string data and require the caller to copy the data to somewhere better, and pray there isn't any race condition.

Description

Change the functions to return std::string and std::wstring objects instead, to get automatic lifetime management of the data, and avoid data sharing between threads.

Additionally, this removes mostly any pretense that we can still build Windows 95 versions of the game.

Limitations

Several of these changes are done blind, so far. Only tested in a single configuration on Windows, the changes for other OS may not compile.

There are unrelated changes here that should probably be split off.

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')
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
@nielsmh nielsmh force-pushed the fix-8713 branch 3 times, most recently from e8eda83 to a9dcb52 Feb 21, 2021
src/os/unix/unix.cpp Outdated Show resolved Hide resolved
@nielsmh
Copy link
Contributor Author

@nielsmh nielsmh commented Feb 21, 2021

Redid the UNICODE removal in #8720 instead.

src/debug.cpp Show resolved Hide resolved
src/os/windows/font_win32.cpp Show resolved Hide resolved
@nielsmh nielsmh marked this pull request as ready for review Apr 2, 2021
@@ -279,7 +279,7 @@ bool FioCheckFileExists(const std::string &filename, Subdirectory subdir)
*/
bool FileExists(const std::string &filename)
{
return access(OTTD2FS(filename.c_str()), 0) == 0;
return access(OTTD2FS(filename).c_str(), 0) == 0;
Copy link
Member

@LordAro LordAro Apr 6, 2021

Choose a reason for hiding this comment

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

I'm concerned about the correctness of OTTD2FS(..).c_str() and it's use of the temporary std::string object. Is that definitely all correct?

Copy link
Contributor

@JGRennison JGRennison Apr 6, 2021

Choose a reason for hiding this comment

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

In this case the temporary will not be destructed until the end of the statement (after access has returned), so it is OK.

Copy link
Member

@LordAro LordAro Apr 6, 2021

Choose a reason for hiding this comment

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

How about all the others? :)

Copy link
Contributor Author

@nielsmh nielsmh Apr 6, 2021

Choose a reason for hiding this comment

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

I went over the changes and checked, yes all the cases of OTTD2FS(x).c_str() are safe: The C string returned is only expected to live until the called function has returned. All the cases where it caused problems the temporary string returned from the conversion function is stored into a proper local variable to manage its lifetime.

LordAro
LordAro approved these changes Apr 6, 2021
@nielsmh nielsmh merged commit 746f1ca into OpenTTD:master Apr 7, 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

4 participants