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

avoid segfault on OpenBSD by not accessing array at index -1 #2097

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

rfht
Copy link
Contributor

@rfht rfht commented Aug 30, 2024

I have encountered reproducible segfaults on OpenBSD's love (version 11.5) when using it to run the game Arco, occurring some way into the second act in the NPC party member inventory:

(gdb) bt
#0  0x00000111a6befa07 in love::graphics::Polyline::render(love::Vector2 const*, unsigned long, unsigned long, float, float, bool) ()
   from /usr/local/lib/liblove-11.5.so.0.0
#1  0x00000111a6bda0d5 in love::graphics::Graphics::polyline(love::Vector2 const*, unsigned long) () from /usr/local/lib/liblove-11.5.so.0.0
#2  0x00000111a6c24ed4 in love::graphics::w_line(lua_State*) ()
   from /usr/local/lib/liblove-11.5.so.0.0
#3  0x0000011115378146 in lj_BC_FUNCC ()
   from /usr/local/lib/libluajit-5.1.so.1.0
#4  0x0000010ecd83f6d8 in ?? ()
#5  0x0000010ecd83f1db in ?? ()
#6  0x0000000000000000 in ?? ()

Adding debug symbols showed it happening at https://github.com/love2d/love/blob/main/src/modules/graphics/Polyline.cpp#L105. When I add a printf right above the line, it reveals that this happens when vertex_count is 0, so this tries to access vertices[-1] in that case.

With the diff in this PR that checks that vertex_count is greater than 0, I've run Arco without any recurrence of the issue, and no new issues with graphics rendering or other. No issues with a few other love 11.5 games that I tested it with either.

I am not sure about the logic behind vertices and vertex_count, so would appreciate a look if this is correct to avoid the array access with negative index...

@rfht
Copy link
Contributor Author

rfht commented Oct 21, 2024

bump
The same thing also happens in Moonring and also seems resolved with this diff. Could I get an opinion if this looks okay to consider merging it?

@slime73
Copy link
Member

slime73 commented Oct 22, 2024

Looks good, thanks! I might want to take a closer look at doing an early return when there are no vertices, but this doesn't hurt in the meantime.

@slime73 slime73 merged commit b2785df into love2d:main Oct 22, 2024
7 checks passed
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.

2 participants