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: Possible double path separator in FiosMakeFilename #7699

Merged
merged 1 commit into from Oct 25, 2019

Conversation

j-pet
Copy link
Contributor

@j-pet j-pet commented Aug 17, 2019

Steps to reproduce: set a breakpoint after the FiosMakeFilename call, then save the game and check the path separator in front of the filename.

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.

Ideally this would all be converted to std::string, but that's a little further off...

src/fios.cpp Outdated Show resolved Hide resolved
src/fios.cpp Show resolved Hide resolved
src/fios.cpp Outdated Show resolved Hide resolved
LordAro
LordAro previously approved these changes Sep 2, 2019
@LordAro
Copy link
Member

LordAro commented Oct 7, 2019

I've been looking at this again, and I wonder if it's actually FiosMakeFilenames "responsibility" to check for a trailing / on the path parameter. I think it's actually FiosBrowseTo and the respective FiosItem's responsibility to remove it (or indeed, not add it in the first place). Thoughts?

Also, checking for the UTF8-part is all well and good, but none of the other code supports it (plenty of strrchr + PATHSEP usages), so it's probably not worth the extra complexity at this time - you're quite welcome to do the whole thing though, but I suspect that's a much larger job :)

@j-pet
Copy link
Contributor Author

j-pet commented Oct 8, 2019

  1. I think that FiosMakeFilename is responsible for handling slashes.
    Removing a trailing slash in FiosBrowseTo and the corresponding FiosItem is undesirable, because _fios_path is used to display on the screen.
    _fios_path

  2. In fact, there is no need to check for the UTF-8 Part when searching for PATHSEPCHAR in the UTF-8 string, because the MSB of characters / \ is 0, and the MSB of any byte of the multibyte character is 1.

My bug (fixed in the push below): In my PR the 'null pointer reference' will occur if the path is nullptr although original function allows it.

LordAro
LordAro previously approved these changes Oct 21, 2019
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.

Personally, I dislike the trailing / / \ of directories, but I accept that some people like the distinction :)

src/fios.cpp Outdated Show resolved Hide resolved
@LordAro LordAro merged commit 8c6a16b into OpenTTD:master Oct 25, 2019
@j-pet j-pet deleted the fix branch October 26, 2019 09:46
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

3 participants