Closed Wohlstand closed 4 years ago
Original comment by jimbo1qaz (Bitbucket: jimbo1qaz, ).
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;
}
Original comment by jimbo1qaz (Bitbucket: jimbo1qaz, ).
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?)
Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).
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.
Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).
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!
Original comment by jimbo1qaz (Bitbucket: jimbo1qaz, ).
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().blip_long
for consistent behavior on 32/64 Windows/Linux.blip_nclock_t
and blip_nsamp_t
(my preferences and use case).
Original report by jimbo1qaz (Bitbucket: jimbo1qaz, ).
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 clearinput.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.
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).