Sfml: Spamming sf::Music::openFromFile() causes division by zero error in SongFileReaderOgg::seek()

[stackyjoe]

Subject of the issue

Spamming calls to sf::Music::openFromFile() is causing SongFileReaderOgg::seek() to divide by zero.

Your environment

• FreeBSD 12.0-RELEASE
• SFML 2.5.1
• clang++ 6.0.1
• -pthread -Wl,-rpath,/usr/local/lib/qt5 -o $files -L/lib -lsfml-audio -lopenal -lFLAC -lvorbisenc -lvorbisfile -lvorbis -logg -ltag -L/usr/local/lib/qt5 -lQt5Widgets -lQt5Gui -lQt5Core -lkvm -lprocstat -L/usr/local/lib -lGL
  

  Steps to reproduce

I'm writing a Qt5 music player application that uses SFML to play music files. In testing it for "robustness" I've found that if I spam calls to sf::Music::openFromFile(), it causes a division by zero error.

Here's the lldb backtrace:

frame #0: 0x0000000800285576 libsfml-audio.so.2.5 ___lldb_unnamed_symbol66$$libsfml-audio.so.2.5 + 22
frame #1: 0x00000008002805a8 libsfml-audio.so.2.5 sf::InputSoundFile::seek(sf::Time) + 120
frame #2: 0x000000080027db20 libsfml-audio.so.2.5 sf::Music::onSeek(sf::Time) + 48
frame #3: 0x00000008002828e6 libsfml-audio.so.2.5 sf::SoundStream::stop(void) + 70
frame #4: 0x000000080027d373 libsfml-audio.so.2.5 sf::Music::openFromFile(std::__1::basic_string<char, sf::Music::openFromFile::char_traits<char>, sf::Music::openFromFile::allocator<char> > const&) + 19
frame #5: 0x0000000000222f8c lamothe sfml_player::openFromFile(this=0x00007fffffffe7e0, path=0x0000000802c448f0) at sfml_player.cpp:42

the top frame is

frame #0: 0x0000000800285576 libsfml-audio.so.2.5 ___lldb_unnamed_symbol66$$libsfml-audio.so.2.5 + 22
libsfml-audio.so.2.5 ___lldb_unnamed_symbol66$$libsfml-audio.so.2.5:
-> 0x800285576 <+22>: divq %rcx
0x800285579 <+25>: movq %rax, %rsi
0x80028557c <+28>: jmp 0x800289a20 ; symbol stub for: ov_pcm_seek
0x800285581 <+33>: pushq %rbp

I think somehow SoundFileReaderOgg::seek() is being called before m_channelCount is set to a non-zero value.

[eXpl0it3r]

If you have a question or experiencing some issue, please ask for help on the forum: https://en.sfml-dev.org/forums/index.php#c3

[stackyjoe]

Sorry I hit enter by accident just after opening the issue editor. I'm typing it up now.

[stackyjoe]

The easiest solution IMO is for the library user to just add a mutex controlling access to such calls. But perhaps a warning somewhere that SFML will let you shoot your own foot would help :)

[eXpl0it3r]

Do you have a minimal and complete example that reproduces the issue?

If you call openFromFile() then seek() shouldn't be called at the same time, so there shouldn't exist a race condition, unless you're applying some additional threading that shares the sf::Music object?

[stackyjoe]

Yes, it's being called from UI interactions which happens in a Qt thread.

[eXpl0it3r]

If you have a shared object between more than one thread, it's your responsibility to properly protect the access to the shared object. SFML makes no multi-threading guarantees, as such you have to ensure that play() isn't being called before openFromFile() has finished. 😉

[eXpl0it3r]

Do you have some example code that we could reproduce the issue with? Otherwise, I'll have to assume that this is related to not properly synchronizing shared resources in a multi-threaded environment.

[dmitry-t]

I think it's worth stating in the documentation that the synchronization is users' responsibility.
I believe it's been stated somewhere already, but I was unable to find this information quickly, and this is not a good sign.

[eXpl0it3r]

Given that none of the SFML functions are thread-safe, it's kind of impossible to add a note to every function, do you have an idea where it would be best placed? Where were you looking at first?

In C++ the usual approach to thread-safety is, that as long as it isn't explicitly specified to be thread-safe, it's to be assumed that it isn't. Mainly for the reason, that to make something thread-safe requires quite a bit of work, so if someone spends a few hours to get that guarantee they'll also make sure to specify that.

[dmitry-t]

I looked e.g. here:

• https://www.sfml-dev.org/documentation/2.5.1/classsf_1_1Music.php
• https://www.sfml-dev.org/documentation/2.5.1/
• https://www.sfml-dev.org/tutorials/2.5/audio-sounds.php
• https://www.sfml-dev.org/faq.php

There is a generic description of synchronization here, but it says how objects should be protected and nothing about which _SFML_ objects should be protected:

• https://www.sfml-dev.org/tutorials/2.5/system-thread.php#protecting-shared-data

[eXpl0it3r]

I opened an issue on the SFML-Website repo, so we can track it there. And if you have some spare time, feel free to provide a pull request. 🙂

I'll close this issue, as it seems to just be an issue with unprotected shared data.