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

Prevent segfault due to division by zero in music.get_pos #2426

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

oddbookworm
Copy link
Member

@pmp-p reported an issue in discord where this code can crash due to a division by zero:

import pygame
pygame.init()
pygame.mixer.init()
pygame.mixer.music.get_pos()

I tried to write a regression test for this fix, but I couldn't reliably reproduce it in a test environment. Because the variables involved are static to the music.c file, I believe this is only possible before any music has ever been loaded, and because other tests run before the created test, those static variables retain their last value, even if pygame.mixer.quit() or pygame.quit() has been called and mixer has been reinitialized

@bilhox reported that they didn't get the floating point exception on windows 11 (pmp-p and I are both using linux), but I suspect that is windows doing windows things and not actually showing the parachute info.

For reference, here is the parachute dump I get:

pygame-ce 2.4.0.dev1 (SDL 2.28.2, Python 3.11.3)
Fatal Python error: pygame_parachute: (pygame parachute) Floating Point Exception
Python runtime state: initialized

Current thread 0x00007f55234a3740 (most recent call first):
  File "/home/andrew/Projects/pygame-ce/test.py", line 4 in <module>

Extension modules: pygame.base, pygame.constants, pygame.rect, pygame.rwobject, pygame.surflock, pygame.bufferproxy, pygame.math, pygame.surface, pygame.display, pygame.draw, pygame.joystick, pygame.event, pygame.imageext, pygame.image, pygame.key, pygame.mouse, pygame.time, pygame.mask, pygame.pixelcopy, pygame.transform, pygame.font, pygame.mixer_music, pygame.mixer, pygame.scrap, pygame.system, pygame._freetype (total: 26)
zsh: IOT instruction (core dumped)  /home/andrew/Projects/pygame-ce/venv/bin/python 

And the gdb result on receipt of the exception

Thread 1 "python" received signal SIGFPE, Arithmetic exception.
0x00007ffff626da71 in music_get_pos (self=<optimized out>, _null=<optimized out>) at src_c/music.c:258
258         ticks = (long)(1000 * music_pos /

@oddbookworm oddbookworm added mixer.music pygame.mixer.music bugfix PR that fixes bug labels Aug 29, 2023
@oddbookworm oddbookworm requested a review from a team as a code owner August 29, 2023 02:28
Copy link
Member

@pmp-p pmp-p left a comment

Choose a reason for hiding this comment

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

what about returning -1 instead of raising

@ankith26 ankith26 added this to the 2.3.2 milestone Aug 29, 2023
@ankith26
Copy link
Member

I also think returning a -1 is more consistent with existing code that already seems to return a -1

@oddbookworm
Copy link
Member Author

Updated

Copy link
Member

@pmp-p pmp-p left a comment

Choose a reason for hiding this comment

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

works fine for me

docs/reST/ref/music.rst Outdated Show resolved Hide resolved
@ankith26 ankith26 changed the title Prevent division by zero in music.get_pos Prevent segfault due to division by zero in music.get_pos Aug 30, 2023
@bilhox
Copy link
Contributor

bilhox commented Aug 30, 2023

By returning -1, does it raise an error now ? Because I still have the same result as before (nothing printed).

@ankith26
Copy link
Member

It doesn't raise an error. The code in the original comment prints nothing now because that is what is expected. Modifying the reproducer a bit to

import pygame
pygame.init()
pygame.mixer.init()
print(pygame.mixer.music.get_pos())

Should print a -1 after this PR. Before this PR, on systems where the segfault message isn't explicitly put into stderr for whatever reason, the program would still abruptly exit before printing anything so this PR is still a fix.

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

The code and doc changes LGTM, thanks for the PR 🎉

I also just remembered, a simple unittest addition would be nice, but 'm approving this PR nonetheless

@bilhox
Copy link
Contributor

bilhox commented Aug 30, 2023

The program still crashes in my side. I'm on Windows 11, perhaps I setup wrongly pygame-ce (I'm using oddbookworm's version).

@oddbookworm
Copy link
Member Author

The program still crashes in my side. I'm on Windows 11, perhaps I setup wrongly pygame-ce (I'm using oddbookworm's version).

You should be able to grab a wheel from the CI runs and install the wheel to check it

@oddbookworm
Copy link
Member Author

I also just remembered, a simple unittest addition would be nice

I agree, but as said in my first message, I couldn’t get it to work. If you have a suggestion to resolve the issue I was seeing with unit testing, I’ll happily write a test

@pmp-p pmp-p merged commit 9e6133a into pygame-community:main Aug 30, 2023
32 checks passed
ankith26 pushed a commit that referenced this pull request Sep 2, 2023
@oddbookworm oddbookworm deleted the music_arithmetic_fault branch December 21, 2023 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug mixer.music pygame.mixer.music
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants