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 invalid file path handling in Windows when there is dot in the file name #88144

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

GNSS-Stylist
Copy link
Contributor

@GNSS-Stylist GNSS-Stylist commented Feb 9, 2024

This basically re-adds the dot-removal removed by the previous commit (PR #88129). It looks like get_basename() (that was meant to replace the old "dot-removal" part?) doesn't actually remove the dot from the filename. Therefore "con.png" etc. becomes "con." and this still froze the editor.

Not sure if the dot-removal should be actually done in get_basename() instead(?) If so, feel free to discard this PR.

@bruvzg

@GNSS-Stylist GNSS-Stylist requested a review from a team as a code owner February 9, 2024 16:25
@AThousandShips
Copy link
Member

AThousandShips commented Feb 9, 2024

This is exactly what the get_basename method does:

godot/core/string/ustring.cpp

Lines 4800 to 4807 in 94dbf69

String String::get_basename() const {
int pos = rfind(".");
if (pos < 0 || pos < MAX(rfind("/"), rfind("\\"))) {
return *this;
}
return substr(0, pos);
}

Are you sure there's nothing else missing here? This shouldn't be needed

Also with get_file there are no slashes, so this should always remove the period

godot/core/string/ustring.cpp

Lines 4692 to 4699 in 94dbf69

String String::get_file() const {
int sep = MAX(rfind("/"), rfind("\\"));
if (sep == -1) {
return *this;
}
return substr(sep + 1, length());
}

This code would also change foo.bar.baz into foo (which might be desired, but it's not the bug mentioned)

The current code correctly turns con.png into con

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Therefore "con.png" etc. becomes "con." and this still froze the editor.

I can't reproduce it, it's correctly returning "con" in this case.

In addition to being redundant, this logic is wrong since it's searching for the dot from the left instead of doing it from the right (like get_basename do), so it will reject a completely valid con.con.con file for example.

@bruvzg
Copy link
Member

bruvzg commented Feb 9, 2024

In addition to being redundant, this logic is wrong since it's searching for the dot from the left instead of doing it from the right (like get_basename do), so it will reject a completely valid con.con.con file for example.

Actually, according to the Windows docs, con.con.con should not be valid, so searching from the lest might be more correct. But names like this seems to work in practice (at least on Windows 11).

Also avoid these names followed immediately by an extension; for example, NUL.txt and NUL.tar.gz are both equivalent to NUL.

@GNSS-Stylist
Copy link
Contributor Author

So you commented faster than me, but I will post this anyway... :)

Did some more testing with debug-prints:

bool FileAccessWindows::is_path_invalid(const String &p_path) {
	// Check for invalid operating system file.

	WARN_PRINT("p_path: " + p_path);

	String fname = p_path.get_file().get_basename().to_lower();

	WARN_PRINT("fname after get_basename(): " + fname);

	int dot = fname.find(".");
	if (dot != -1) {
		fname = fname.substr(0, dot);
	}

	WARN_PRINT("fname after manual dot removal: " + fname);

	return invalid_files.has(fname);
}

Output with BBCode "[img]con.[/img]"
drivers\windows\file_access_windows.cpp:64 - p_path: res://con..remap
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con.
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
drivers\windows\file_access_windows.cpp:64 - p_path: res://con..import
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con.
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
Resource file not found: res://con. (expected type: Texture2D)

BBCode "[img]res://con.png[/img]":
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.png.remap
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con.png
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.png.import
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con.png
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.png
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
Resource file not found: res://con.png (expected type: Texture2D)

BBCode "[img]res://con.png.tmp[/img]":
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.png.tmp.remap
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con.png.tmp
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.png.tmp.import
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con.png.tmp
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.png.tmp
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con.png
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
Resource file not found: res://con.png.tmp (expected type: Texture2D)

BBCode "[img]con.png[/img]":
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.png.remap
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con.png
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.png.import
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con.png
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
drivers\windows\file_access_windows.cpp:64 - p_path: res://con.png
drivers\windows\file_access_windows.cpp:68 - fname after get_basename(): con
drivers\windows\file_access_windows.cpp:75 - fname after manual dot removal: con
Resource file not found: res://con.png (expected type: Texture2D)

Those ".remap" and ".import" extensions confure me a bit. And I was wrong about ""con.png" etc. becomes "con."". Not sure why I came to that conclusion...

Reverted my changes and tried with some strings (same as above + "bare" con):
"[img]res://con.[/img]" -> Freeze
"[img]res://con.png[/img]" -> Freeze
"[img]res://con.png.tmp[/img]" -> Freeze
"[img]con.png[/img]" -> Freeze
"[img]con[/img]" -> Works

None of these freeze the editor with this PR. PC has windows 10.

@akien-mga
Copy link
Member

akien-mga commented Feb 12, 2024

Looks good! Could you squash the commits? See PR workflow for instructions.

Edit: Done myself.

…le name

This basically re-adds dot-removal removed by the previous commit.
@akien-mga akien-mga changed the title Fix invalid file path handling in windows when there is dot in the file name Fix invalid file path handling in Windows when there is dot in the file name Feb 13, 2024
@akien-mga akien-mga merged commit d3a8ae8 into godotengine:master Feb 13, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@GNSS-Stylist GNSS-Stylist deleted the ConDotFix branch February 15, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants