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 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 left a comment

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
@j-pet j-pet force-pushed the j-pet:fix branch from 3c23cc7 to 66ce92e Sep 1, 2019
src/fios.cpp Outdated Show resolved Hide resolved
@LordAro

This comment has been minimized.

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

This comment has been minimized.

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.

@j-pet j-pet force-pushed the j-pet:fix branch from 66ce92e to 8d0abd9 Oct 8, 2019
Copy link
Member

LordAro left a comment

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

src/fios.cpp Outdated Show resolved Hide resolved
@j-pet j-pet force-pushed the j-pet:fix branch from 8d0abd9 to a13f4c3 Oct 22, 2019
@LordAro LordAro merged commit 8c6a16b into OpenTTD:master Oct 25, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20191022.3 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@j-pet j-pet deleted the j-pet:fix branch Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.