Skip to content

Improve error messages for invalid resources imported in the editor #106734

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented May 22, 2025

The following situations will now print bespoke error messages:

  • Importing an empty resource (0 bytes). This is generally a corrupted file (written during a crash or power outage) where the OS leaves an empty file.
  • Importing a Git LFS pointer (which always starts with the same first line) instead of the actual file data. Running git lfs pull replaces this pointer with the actual file data.

Testing project: test_invalid_files.zip

Preview

WARNING: Not a PNG file
     at: check_error (./drivers/png/png_driver_common.cpp:55)
ERROR: Condition "!success" is true. Returning: ERR_FILE_CORRUPT
   at: png_to_image (./drivers/png/png_driver_common.cpp:68)
ERROR: Error loading image: 'res://actually_a_jpg.png'.
   at: load_image (./core/io/image_loader.cpp:100)
ERROR: Error importing 'res://actually_a_jpg.png'.
   at: _reimport_file (./editor/editor_file_system.cpp:3024)
ERROR: Resource file is a Git LFS pointer (run `git lfs pull` to fetch it): res://lfs_pointer.png
   at: _reimport_file (./editor/editor_file_system.cpp:2767)
ERROR: Resource file is corrupt (empty size): res://empty.png
   at: _reimport_file (./editor/editor_file_system.cpp:2751)

The following situations will now print bespoke error messages:

- Importing an empty resource (0 bytes). This is generally a corrupted
  file (written during a crash or power outage) where the OS leaves
  an empty file.
- Importing a Git LFS pointer (which always starts with the same first line)
  instead of the actual file data. Running `git lfs pull` replaces
  this pointer with the actual file data.
Comment on lines +2763 to +2767
const uint8_t first_byte = file->get_8();
const uint8_t second_byte = file->get_8();
const uint8_t third_byte = file->get_8();
const uint8_t fourth_byte = file->get_8();
if (first_byte == 'v' && second_byte == 'e' && third_byte == 'r' && fourth_byte == 's') { // "vers" in "version"
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a similar check in place in the OBJ importer, although it's designed to check if the file is binary instead of being text: https://github.com/godotengine/godot/blob/master/editor/import/3d/resource_importer_obj.cpp#L204

It's likely possible to write this in a more robust/efficient manner, but it probably works well enough as-is.

Copy link
Member

Choose a reason for hiding this comment

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

It's a simple ASCII string, so it might be better to just check it in the loop instead of using get_line(), which would still print errors if file start with vers` and have invalid UTF-8 sequences.

	const char *sample = "version https://git-lfs";
	bool match = true;
	for (int i = 0; sample[i] != 0; i++) {
		if (f->eof_reached() || sample[i] != f->get_8()) {
			match = false;
			break;
		}
	}
	ERR_FAIL_COND_V_MSG(match, ERR_FILE_UNRECOGNIZED,
				vformat("Resource file is a Git LFS pointer (run `git lfs pull` to fetch it): %s", p_file));

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.

2 participants