6 ms·
I spent some time trying to figure out why the lock-free read/write implementation is correct under x86, assuming a multiprocessor environment. My read of the
by vitus 5y ago
I spent some time trying to figure out why the lock-free read/write implementation is correct under x86, assuming a multiprocessor environment.
My read of the situation was that there's already potential for a double-read / double-write between when the spinlock returns and when the head/tail index is updated.
Turns out that I was missing something: there's only one producer thread, and only one consumer thread. If there were multiple of either, then this code would be more fundamentally broken.
That said: IMO the use of `new` in modern C++ (as is the case in the writer queue) is often a code smell, especially when std::make_unique would work just as well. Using a unique_ptr would obviate the first concern [0] about the copy constructor not being deleted.
(If we used unique_ptr consistently here, we might fix the scary platform-dependent leak in exchange for a likely segfault following a nullptr dereference.)
One other comment: the explanation in [1] is slightly incorrect:
> we receive back Result* pointers from the results queue rq, then wrap them in a std::unique_ptr and jam them into a vector.
We actually receive unique_ptrs from the results queue, then because, um, reasons (probably that we forgot that we made this a unique_ptr), we're wrapping them in another unique_ptr, which works because we're passing a temporary (well, prvalue in C++17) to unique_ptr's constructor -- while that looks like it might invoke the deleted copy-constructor, it's actually an instance of guaranteed copy elision. Also a bit weird to see, but not an issue of correctness.
[0] https://github.com/stong/how-to-exploit-a-double-free#0-internal-data-structures https://github.com/stong/how-to-exploit-a-double-free#0-inte...
[1] https://github.com/stong/how-to-exploit-a-double-free#2-receive-results https://github.com/stong/how-to-exploit-a-double-free#2-rece...
- PaulDavisThe1st 5y ago> Turns out that I was missing something: Indeed. It's not safe under x86 either.
- stong1 5y agoGreat points. I made some minor edits to address that and clarify some things. Thanks!
- aydwi 5y ago> IMO the use of `new` in modern C++ (as is the case in the writer queue) is often a code smell As a naive practitioner of modern C++, I'd love it if you could elaborate on this.
- ddlutz 5y agoIt's widely accepted you should almost always use some smart pointer like unique_ptr or shared_ptr, and even sometimes the the unpopular weak_ptr depending on your use case.
- usefulcat 5y agoWhenever you use 'new', you have to decide what is going to 'own' the newly allocated thing. You'll also have to remember to call 'delete' somewhere. Using unique_ptr/make_unique() or shared_ptr/make_shared() automates lifetime management (obviates the need for a manual 'delete') and makes the ownership policy explicit. They also have appropriately defined copying behavior. For example: struct Foo { // lots of stuff here ... }; struct A { Foo* f = new Foo; ~A() { delete f; } }; struct B { std::unique_ptr<Foo> f = std::make_unique<Foo>(); // no need to define a dtor; the default dtor is fine }; For the destructor and the default constructor, compilers will generate basically identical code for both A and B above. If you try to copy a B, the compiler won't let you because unique_ptr isn't copyable. However it won't stop you from copying an A, even though as written (using the default copy ctor) that's almost certainly a mistake and will likely result in a double free in ~A().
- vitus 5y agoExactly as usefulcat points out: with modern C++, the bulk of object lifetimes should be handled with unique_ptr and/or directly on the stack, so your destructors are automatically called for you (reducing the risk of double frees or memory leaks). unique_ptr forces you to think about your dependencies and when objects can / should be cleaned up.