SoLoud::Wav does not have proper copy ctor, etc.

[klein-j]

When a SoLoud::Wav object is copied, a shallow copy is done. This can lead to deleting m_data twice, which crashes.

Fix: Have proper Copy-Constructor and assignment operator, or mark them deleted / private to avoid misuse. This might affect several other classes as well.

Example Code:

#include <stdlib.h>

#define WITH_XAUDIO2

#include "soloud.h"
#include "soloud_speech.h"
#include "soloud_thread.h"
#include "soloud_wav.h"

// Entry point
int main(int argc, char *argv[])
{
    // Define a couple of variables
    SoLoud::Soloud soloud; 
    SoLoud::Wav sound;

    sound.load("test.wav");
    sound.setVolume(1);
    {
    //  SoLoud::Wav sound2 = sound;
    }
    soloud.init();

    // Play the sound source (we could do this several times if we wanted)
    soloud.play(sound);
    while(soloud.getActiveVoiceCount() > 0)
    {
        SoLoud::Thread::sleep(100);
    }
    soloud.deinit();
    return 0;
}

If SoLoud::Wav sound2 = sound; is uncommented, the programm will crash.

Version: soloud20200207

[klein-j]

Making float *mData an std::unique_ptr should be a good idea as well. Then it should become directly obvious, when the Sound-Object is attempted to be copied without an proper copy ctor / assignment operator.

From the CPP core guidelines: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-ptr

[klein-j]

No, actually I think float *mData really should be std::vector<float> mData.

The changes are small and it instantaneously fixed all crashes. I will prepare a pull-request.

Still for efficiency reasons, proper cpy-constr and assignment-operators probably should be implemented. However I'd take a needless copy of a small sound buffer a hundred times over the application just crashing, so this is already a major improvement.

[klein-j]

As I said, I can help fixing this issue by providing some code, but first we should have at least some form of discussion to ensure that a fix will actually be in line with the project and can be merged, once ready.

[klein-j]

Here are the diff files. Renamed as txt for github upload, also the index in line 2 is relative to my own repository, so maybe they can't be applied directly.

soloud_wav.cpp.txt soloud_wav.h.txt

[apples]

Just ran into this issue as well. I think m_data should be a unique_ptr instead of a vector, since it closer matches the current behavior/assumptions. It would remove the need for a destructor, and allow Wav to be used in data structures that support move-only types, such as vector<Wav>.

@jarikomppa Can we get some discussion about this? Seems like a pretty trivial issue that leads to easy crashes.

[qwiff-dogg]

Bump. I ran into this issue as well. The out-of-the-box experience of Wav is not good and the proposed solutions are "low-hanging fruit" improvements.

[jarikomppa]

@jarikomppa Can we get some discussion about this? Seems like a pretty trivial issue that leads to easy crashes.

The structures are not meant to be copied. I don't quite see the point. Of course, if there's a clear use case, I'd consider it.

At the moment though, if something, I'd add copy-blocking constructors, but I don't feel adding such things adds value. Next you'd probably be complaining about all the public methods and lack of accessors, and maybe the lack of modern c++ features..

[qwiff-dogg]

The structures are not meant to be copied. I don't quite see the point. Of course, if there's a clear use case, I'd consider it.

At the moment though, if something, I'd add copy-blocking constructors, but I don't feel adding such things adds value. Next you'd probably be complaining about all the public methods and lack of accessors, and maybe the lack of modern c++ features..

If these structures are not meant to be copied, then I'd say it's better to leave things as-is. Simply noting that them being non-copyable is a design decision would be sufficient for me.

[klein-j]

Honestly, what really "does not add value" is having a copy-constructor that is implemented wrongly and crashes the program. This is bad, it either has to be removed or fixed. Granted, the bad copy-ctor is implicitly created by C++, it is one of the things lengthy discussions why C++ sucks are about. But this does not change the fact that something that should never be used, should not be provided in the first place. If it is implicitly generated and should not exist, the right solution is to explicitly delete it (WavInstance(const WavInstance&) = delete;). There is no point in writing "just don't copy these objects" somewhere in the documentation where nobody sees it, if you can just write it very clearly in the code and have the compiler automatically warn the user, if he does something wrong.

However, I still think that doing your own memory management is not a good idea, and the solution with the vector is better (less code, less bugs => easier to understand and more stable to use). However I see how this is debatable.

[apples]

The structures are not meant to be copied. I don't quite see the point.

They don't need to be copyable, they just need to have a (probably deleted) copy constructor.

Move constructors (which set mData to null) are also necessary here.

At the moment though, if something, I'd add copy-blocking constructors, but I don't feel adding such things adds value.

List of things that currently cause a crash:

• Returning a Wav from a function without RVO.
• Using a Wav in any standard library container (especially std::vector).
• Assigning myWav = Wav();.
• Creating a new struct like struct Thing { Wav myWav; }; and then doing any of the above with that struct.

I feel like these are all really basic, common things to want to do with pretty much any type, so for me personally it would add a ton of value.

Not to mention the value of time saved tracking down weird crashes.

As it stands, I feel obligated to always wrap Wav in a std::shared_ptr or something to "fix" these issues.

Next you'd probably be complaining about all the public methods and lack of accessors, and maybe the lack of modern c++ features..

Nobody in this thread has mentioned any of those things. This isn't even "modern C++", this whole discussion applies to C++03 and prior versions as well.

[klein-j]

I also uploaded my fix here: https://github.com/klein-j/soloud_fixed/tree/soloud_fixed