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

Waving sprites with a width and height of 0 crash mkxp-z #189

Open
melody-rs opened this issue May 23, 2024 · 3 comments · May be fixed by #147
Open

Waving sprites with a width and height of 0 crash mkxp-z #189

melody-rs opened this issue May 23, 2024 · 3 comments · May be fixed by #147

Comments

@melody-rs
Copy link

melody-rs commented May 23, 2024

This is an issue present in mkxp-z but not modshot. I can't for the life of me figure out why, but when SpritePrivate::updateWave() is called and the sprite width and height is 0, this line of code resizes wave.qArray.vertices to a size of 0, which then immediately crashes mkxp-z. This isn't an issue present in modshot (I haven't tested mkxp) and I can't find any significant differences in sprite.cpp that'd cause this.

image
(log from when i was checking resizes)

Resizing to  4  chunks 
Resizing to  4  chunks 
Resizing to  4  chunks 
Resizing to  4  chunks 
Resizing to  0  chunks 
/usr/include/c++/13.2.1/bits/stl_vector.h:1128: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = SVertex; _Alloc = std::allocator<SVertex>; reference = SVertex&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
./oneshot: line 4: 616617 Aborted                 (core dumped) ./oneshot

A pretty hacky fix can be done to fix the bug:

if (width == 0 || height == 0)
{
    wave.active = false;
    return;
}

I don't trust the fix though as it's not really addressing why there's a difference in behavior between modshot and mkxp-z

@WaywardHeart
Copy link

That fix looks correct to me. Accessing an entry past the end of a vector causes undefined behavior. Either you're getting (un)lucky and your build of mkxp-z is consistently trying to access memory it shouldn't at that point, or your compiler is being more strict than theirs, assuming you didn't build modshot yourself, that is.

@melody-rs
Copy link
Author

melody-rs commented Jun 2, 2024

¯\_(ツ)_/¯

I did build modshot myself? Weird

@thehatkid
Copy link

I have reproduced waving effect on Sprite object that has 0 width or/and 0 height by using spr_obj.src_rect = Rect.new(0, 0, 0, 0).
It only appears when mkxp-z (ModShot in our case) was built in Debug mode with Meson build system --buildtype=debug argument. On Release mode, it does nothing (nor uncaught exception or crash).
However, it is looks unsafe though, attempting to get item index that may be not there.

thehatkid added a commit to thehatkid/ModShot-mkxp-z that referenced this issue Aug 3, 2024
Resolves mkxp-z/mkxp-z#189
Fixes C++ exception (assertion fail) when getting first item from `wave.qArray.vertices`.
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 a pull request may close this issue.

3 participants