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

No nullptr check in cowdata - crash to desktop #81911

Closed
ShlomiRex opened this issue Sep 19, 2023 · 4 comments · Fixed by #81917
Closed

No nullptr check in cowdata - crash to desktop #81911

ShlomiRex opened this issue Sep 19, 2023 · 4 comments · Fixed by #81917

Comments

@ShlomiRex
Copy link
Contributor

Godot version

v4.2.dev.custom_build.ba54c3455

System information

macOS M1

Issue description

In cowdata.h line 314:

*_get_size() = p_size;

We can see:

*_get_size() = p_size;

The function _get_size() may return nullptr:

	_FORCE_INLINE_ uint32_t *_get_size() const {
		if (!_ptr) {
			return nullptr;
		}

		return reinterpret_cast<uint32_t *>(_ptr) - 1;
	}

I discovered this when investigating the issue: #63534

This is the cause of the issue and the cause of the crash.

Since I don't understand Copy-On-Write logic, I don't know how to fix it. So I leave it to you guys.

Steps to reproduce

  1. Get Editing Tileset Causes Crash to desktop #63534 project (download the example)
  2. Open (double click in godot editor)res://godot_4_alpha_12_tileset_issue_2.log
  3. Crash into desktop

Minimal reproduction project

Get #63534 project (download the example)

@AThousandShips
Copy link
Member

I'm not sure how this could happen as all the paths leading to *_get_size() = p_size; are null checked?

@AThousandShips
Copy link
Member

Might know, will do some checking later today

@AThousandShips AThousandShips self-assigned this Sep 19, 2023
@ShlomiRex
Copy link
Contributor Author

I'm not sure how this could happen as all the paths leading to *_get_size() = p_size; are null checked?

I did small change to see the result of _get_size() and indeed it's zero-pointer:

image

@AThousandShips
Copy link
Member

I've identified the error, looking into a fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants