Blip_Buffer move-constructor causes double free of buffer_

[Former user]

I'm writing a NES tracker depending on blip_buffer. I decided to use the Blip_Buffer 0.4.1 found in this game-music-emu repository. This version was never released independently on https://code.google.com/archive/p/blip-buffer/ . (kode54's GME has a heavily modified blip buffer of unknown version, whereas this one is almost identical to blargg's game-music-emu's 0.4.1.)

In 7b29624e96c873a7a0194e2ebe92b97400527d3c, you added a defaulted move constructor Blip_Buffer(Blip_Buffer &&) = default; to blip_buffer.

In my own tracker, I copied Blip_Buffer.cpp/h from this repo to my own source tree, and used it directly. However this move constructor is wrong on MSVC2019, since when moving from the input Blip_Buffer to this, it fails to clear input.buffer_. This leads to a double-free of the buffer, causing a 0xFFFFFFFFFEEEFEEE crash.

Sample code. I added blip2 and blip3 to make the program crash as soon as the function returns, rather than when the returned value is (written to?) or deleted.

Blip_Buffer make_blip_buffer(long smp_per_s, long clk_per_s) {
    Blip_Buffer blip;

    // Set output sample rate and buffer length in milliseconds (1/1000 sec, defaults
    // to 1/4 second), then clear buffer. Returns NULL on success, otherwise if there
    // isn't enough memory, returns error without affecting current buffer setup.

    // 0CC ignores the return code (nonzero char* if error). I guess we'll do that too?
    blip.set_sample_rate(smp_per_s);

    // Set number of source time units per second
    blip.clock_rate(clk_per_s);

    auto blip2 = std::make_unique<Blip_Buffer>(std::move(blip));
    auto blip3 = Blip_Buffer(std::move(*blip2));
    return blip3;
}

Not sure what's the best solution. Override the move constructor and set buffer_ = null? Replace buffer_ with a unique_ptr?

If the move constructor is used by GME, this may be a security issue. Maybe it’s not an issue if you never allocate memory before moving the buffer (so buffer_ is nullptr).

‌

[Former user]

Seems to work if you un-default the move constructor and add in the .cpp:

Blip_Buffer::Blip_Buffer(Blip_Buffer &&other)
    : Blip_Buffer() // initialize via default constructor, C++11 only
{
    memmove(this, &other, sizeof *this);
    other.buffer_ = nullptr;
}

‌

[Former user]

Also Blip_Synth’s defaulted copy and move constructors are incorrect, since the struct is self-referential. Blip_Synth.(Blip_Synth_ impl).impulses points to Blip_Synth.impulses.

Although this type is not movable in Rust (where move is memcpy), in C++ you could implement a copy constructor which modifies the copy so that impl.impulses = impulses. (Since all memory is allocated in-place, there is no benefit in having a move constructor. Maybe it should be deleted?)

[Michael Pyne]

My first instinct is to remove the defaulted move constructor Despite my commit log saying it was to allow to hold in a vector, doesn’t appear to be actually used for that. At least, the library still compiles fine if I remove it, so GME doesn’t actually need it.

[Michael Pyne]

For the Blip_Synth thing, I seem to be able to disable move/copy construction completely without ill effect

‌

diff --git a/gme/Blip_Buffer.h b/gme/Blip_Buffer.h
index e6facc8..e800c7f 100644
--- a/gme/Blip_Buffer.h
+++ b/gme/Blip_Buffer.h
@@ -95,8 +95,6 @@ public:
        Blip_Buffer();
        ~Blip_Buffer();

-       Blip_Buffer(Blip_Buffer &&) = default;
-
        // Deprecated
        typedef blip_resampled_time_t resampled_time_t;
        blargg_err_t sample_rate( long r ) { return set_sample_rate( r ); }
@@ -233,6 +231,10 @@ private:
 public:
        Blip_Synth() : impl( impulses, quality ) { }
 #endif
+
+       // disable broken defaulted constructors, Blip_Synth_ isn't safe to move/copy
+       Blip_Synth<quality, range>(const Blip_Synth<quality, range>  &other) = delete;
+       Blip_Synth<quality, range>(      Blip_Synth<quality, range> &&other) = delete;
 };

 // Low-pass equalization parameters

Does this work in your own testing? If so I’ll go ahead and commit. Thanks for reporting both!

[Former user]

I don’t think I can actually test it. First I’m not using game-music-emu, but rather blip_buffer only. Also my version of blip_buffer has diverged significantly from this repo, and is no longer incompatible.

• Blip_Synth<range> is no longer a template parameter, but an integer parameter to volume().
• I also replaced long with blip_long for consistent behavior on 32/64 Windows/Linux.
• Replaced default constructors with argument constructors (my preferences and use case)
• Added blip_nclock_t and blip_nsamp_t (my preferences and use case).
• Copied Q_DISABLE_MOVE_COPY etc. to Blip_Buffer.h but renamed it BLIP_DISABLE_MOVE_COPY, and used it here. Note that this deletes both copy construction and assignment operators, and you didn’t disable assignment operators!!

‌

[Michael Pyne]

Got it. Either way thanks for the feedback. I’ll go ahead and disable assignment as well and we can work from there. Good luck on your own project!

[Michael Pyne]

Disable copy/move ctor & assignment operator for Blip_Buffer.

This class cannot be safely moved, copied or assigned from so we shouldn't let the compiler try to do this.

Fixes issue #35.

→ <<cset 97527b20a40e>>