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

Playing certain ogg files cause crashes in 2.4.2 that did not in 2.1 #1241

Closed
jamiltron opened this issue Jun 13, 2017 · 9 comments
Closed

Comments

@jamiltron
Copy link

jamiltron commented Jun 13, 2017

Hello,

I am updating an application that uses sfml-audio from 2.1 to 2.4.2 that consistently crashes when playing certain ogg files. These files did not crash in 2.1, and its worth noting that they only crash when loading from stream, but if they are loaded from file they play fine.

Here is an example of an overridden InputStream class that suffers this issue:

#include <SFML/Audio.hpp>
#include <SFML/System.hpp>

class MyStream: public sf::InputStream
{
public:
   MyStream(std::string const& uri);
   sf::Int64 read(void* data, sf::Int64 size) override;
   sf::Int64 seek(sf::Int64 position) override;
   sf::Int64 tell() override;
   sf::Int64 getSize() override;

private:
   std::shared_ptr<std::istream> _stream;
   sf::Int64 _size;
};

MyStream::MyStream(std::string const& path)
{
   std::shared_ptr<std::ifstream> in = std::make_shared<std::ifstream>(path, std::ios::in | std::ios::binary);
   _stream = std::static_pointer_cast<std::istream>(in);
   _stream->seekg(0, std::ios_base::end);
   _size = tell();
   _stream->seekg(0, std::ios_base::beg);
}

sf::Int64 MyStream::read(void* data, sf::Int64 size)
{
   _stream->read((char *)data, size);
   return _stream->gcount();
}

sf::Int64 MyStream::seek(sf::Int64 position)
{
   _stream->seekg(position, std::ios_base::beg);
   return tell();
}

sf::Int64 MyStream::tell()
{
   return _stream->tellg();
}

sf::Int64 MyStream::getSize()
{
   return _size;
}

I believe I am following the guidelines listed here: https://www.sfml-dev.org/tutorials/2.4/system-stream.php

Here is the problematic ogg file. This file passes the oggz validate program.

I should note that I have tried the above changing the implementation slightly - using raw pointers, using a raw filebuf, etc., but all modifications suffer from the same issue. If I set the istreams exceptions and wrap each call in a try/catch, it seems that read fails due to the eof bit being set.

An additional peculiar thing to note is that if I open the problematic file in audacity and save it with varying levels of compression I get different results. Here is the file and its results for each 10 quality levels Audacity allows me to export the file to:

0 quality - program runs, no audio plays
1 quality - same as above
2 quality - program crashes
3 quality - program hangs forever
4 quality - no audio plays
5 quality - program works, audio plays
6 quality - crashes
7 quality - crashes
8 quality - program works, audio plays
9 quality - program works, audio plays
10 quality - program works, audio plays

I know that since 2.1 SFML has removed libsndfile - is it possible that my problem is related to this?

Any help is greatly appreciated!

@mantognini
Copy link
Member

We'll need a truly complete, and minimal, program that reproduce the issue. Also, check what happens with the master version. Further details on how to report bugs are explained in the contribution guidelines. And what happens if you read the ogg file with sf::Music directly? If the bug is present with the master version, check if SFML dependencies can be updated and if it fixes the bug.

@jcowgill
Copy link
Contributor

OK so I've looked at this. Here's a minimal testcase which crashes: https://gist.github.com/jcowgill/4fa0b227e1575e4baec82833a63d1bfa

I've adjusted the testcase to remove the shared_ptr objects to simplify things a bit.

There are 2 bugs here:

  • Your implementation of sf::InputStream::seek is wrong in the case where you attempt to seek after the entire file has been read (which is required by the OGG reader). If you read from an istream past the end of a file, the failbit is set. This will later cause all calls to tellg to return -1 instead of the actual position. You can probably fix this by adding _stream.clear() at the start of MyStream::seek.
  • There appears to be a bug in vorbisfile where if seek fails at a specific time when a file is loaded, it will attempt to free some uninitialized memory and crash. The relevant part is in https://git.xiph.org/?p=vorbis.git;a=blob;f=lib/vorbisfile.c#l1232 (the ov_raw_seek function). If seek_helper on line 1257 (which calls your seek function) fails, then work_os will be freed on 1396 without having been initialized.

@jcowgill
Copy link
Contributor

And here's my vorbis bug for reference: https://trac.xiph.org/ticket/2327

@jamiltron
Copy link
Author

Thank you!

@jamiltron
Copy link
Author

jamiltron commented Jun 14, 2017

Are there any docs on updating sfml's dependencies so I can see if this would remedy my problem? I've tried downloading libogg and libvorbis, applying the patch @jcowgill made, then I built libogg_static.lib, libvorbis_static.lib, and libvorbisfile_static.lib, moved those to the appropriate extlibs directories in SFML, renamed them to match the naming that SFML uses, then built the project, but now I am not hearing any sound. I'm unsure if I have just set something up incorrectly, or if I have built the dependencies wrong, or if something else is going on.

@1aam2am1
Copy link
Contributor

You will have not sound, because your InputStream::seek is wrong. Read jcowgill commend.
Now orbis will not break in error stream. But you have not repair your problem.

@jamiltron
Copy link
Author

jamiltron commented Jun 15, 2017

@1aam2am1 I have added in the clear call within seek as suggested. Additionally - we previous had ogg files that would play. Now none of them play. With/without this suggestion no sound plays at all.

@jcowgill
Copy link
Contributor

I'm not sure if I can help you much here. Adding the call to clear like this (and without changing libvorbisfile), allows you example ogg to play properly for me:

sf::Int64 MyStream::seek(sf::Int64 position)
{
    _stream.clear();
    _stream.seekg(position, std::ios_base::beg);
    return tell();
}

I'm using Debian 9 x86_64. I haven't tried windows.

@mantognini
Copy link
Member

The culprit is read which can set eof/failbit on your stream when reaching EOF. @jcowgill's solution works fine here. 👍

sezero pushed a commit to sezero/tremor that referenced this issue Nov 11, 2018
SFML/SFML#1241
https://trac.xiph.org/ticket/2327
https://gitlab.xiph.org/xiph/vorbis/issues/2327

If _seek_helper fails in ov_raw_seek, control jumps to the seek_error
label which calls ogg_stream_clear on work_os. However, at this point
in the function, work_os is not initialized so we end up attempting to
free some uninitialized memory and crashing.

Fix by removing the call to ogg_stream_clear. This is safe because the
only code path to seek_error happens before work_os is initialized (so
there is never anything to free anyway).

I also refactor the code a bit:
- Remove the ret variable which is unnessesary since we can just pass
  the result of _seek_helper directly to the if.
- Since seek_error is only used once, move the contents of that block
  to the if statement so we can remove a goto.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants