Skip to content

Conversation

@VaderDev
Copy link
Contributor

@VaderDev VaderDev commented Jul 10, 2023

Fix incorrect index calculations in image conversions in ktx create (#733, #736)

@MarkCallow
Copy link
Collaborator

@aqnuep please review.

@MarkCallow
Copy link
Collaborator

Why did the user not receive any notice of the crash?

And, less importantly, why did it produce an output file on macOS?

@aqnuep
Copy link
Collaborator

aqnuep commented Jul 12, 2023

We discussed this change with @vadery. It was a trivial oversight where row stride was incorrectly derived from the height instead of the width.

I'm not sure about the different behaviors across platforms, but I'd expect this typo would mainly only cause data corruption for non-square textures due to the incorrect row stride.

Copy link
Collaborator

@aqnuep aqnuep left a comment

Choose a reason for hiding this comment

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

LGTM.

@aqnuep
Copy link
Collaborator

aqnuep commented Jul 12, 2023

FYI, @MarkCallow, @vadery also added a bunch of odd sized textures to the CTS to have better coverage on this front.

We apologize for the typo, but it's been a huge set of functionalities to cover with implementation and tests, and obviously this slipped through the cracks.

@VaderDev
Copy link
Contributor Author

Why did the user not receive any notice of the crash?

Technically the exit code was an indicator (it was 0xC0000005 = Access Violation). But as it was a bug and not an error that we should ever handle, there was no reasonable way to produce an error message about it.
There are ways with C++ to intercept such crashes (breakpad, crashbed, signals, etc) and attempt to collect more information, write out coredumps and notify the user. But it is not a trivial undertaking to implement or adopt these in a cross platform manner.

And, less importantly, why did it produce an output file on macOS?

Given some bad luck these kind of undefined behaviors are sometimes hidden on certain platform. When I checked in windows builds every compiler yielded a proper crash. Its possible that the application owned the memory pages that was written to and there was nothing important.

@MarkCallow MarkCallow merged commit 682f456 into KhronosGroup:main Jul 13, 2023
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.

3 participants