-2

I thought I was going crazy, but my heap-allocated values are being modified while being passed to cout <<.

To generally describe my setup, I've got a ring buffer containing classes which are basically (value, timestamp) tuples with (double, double) type. I'm attempting to implement a signal delay such that values passed into this buffer are held in the buffer for some fixed delay before finally being emitted. I've initialized a buffer size 1024 of these objects with some known initial condition and then upon receiving a new input value I peek at the head of the list to see if the difference between it's timestamp and the new value's timestamp is greater than the delay. If it is, I pop the head of the list off and discard it. Finally I return the current head of the list.

This all works pretty much as I'd expect it.

If I add a simple print statement (in my main, after invoking the delay filter):

std::cout << "Signal out of delay filter: (" << s.get << ", " << s.get_timestamp() << ").\n";

However, everything explodes. Suddenly elements in my ring buffer are filled with garbage data which breaks my timestamp comparisons.

Any idea? Here's some code snippets that might be relevant, I can post more on request.

edit: I've set a watchpoint on the first entry in my buffer to see where it's modified and it's definitely inside the cout << call. Here's what GDB had to say

Hardware watchpoint 2: d.buf.buffer[0]  

Old value = {value = 0, timestamp = 0} 
New value = {value = 4.10047448604823463e-322, timestamp = 0}
_IO_new_file_overflow (f=0x7ffff783a620 <_IO_2_1_stdout_>, ch=83) at fileops.c:857 857:        
fileops.c: No such file or directory

edit 2: I'm not sure how to properly attach an MVCE, but here's the contents of the files. I hope this is minimal enough, but it captures the problem so it's at least a CVE. If this is too much I can try to cut it down further but I run the risk of eliminating the bug if it gets much more minimal:

CMakeLists.txt:

# Project Definiton

cmake_minimum_required(VERSION 3.5)
project("ring_buffer_mvce")

set_property(GLOBAL PROPERTY CXX_STANDARD 14)
set_property(GLOBAL PROPERTY CXX_STANDARD_REQUIRED ON)

# Automated Unit Test Configuration
enable_testing()

add_executable(delay_test
  src/signals/delay.cpp
  src/test/delay_test.cpp)

add_test(delay_test delay_test)

src/signals/delay.cpp

#include "delay.hpp"

delay::delay(double time, size_t buffer_size, signal<double> initial_condition) {
    this->time = time;
    buf = ring_buffer<signal<double> >(buffer_size);
    for (int i = 0; i < buffer_size; i++) {
        buf.push(initial_condition);
    }
}

delay::~delay() {
}

signal<double> delay::apply(signal<double> s) {
    // Add the current signal to the delay buffer
    buf.push(s);

    // Check to see if our current time has elapsed
    double cur_time_delta = s.get_timestamp() - buf.peek().get_timestamp();
    if (cur_time_delta >= (time - DELAY_EPSILON)) {
        buf.pop();
    }

    return buf.peek();
}

src/signals/delay.hpp:

#ifndef __DELAY_H__
#define __DELAY_H__

#include <cstddef>
#include <limits>
#include "signal.hpp"
#include "filter.hpp"
#include "../utils.h"

#define DELAY_EPSILON 0.00001

class delay : public filter<double> {
    public:
    delay(double time, size_t buffer_size, signal<double> initial_condition);
    ~delay();

    virtual signal<double> apply(const signal<double>);

    private:
    double time;
    ring_buffer<signal<double> > buf;
};

#endif

src/signals/filter.hpp:

#ifndef __FILTER_HPP__
#define __FILTER_HPP__

#include "signal.hpp"

template <class t>
class filter {
    public:
    virtual signal<t> apply(const signal<t>) = 0;
};

#endif

src/signals/signal.hpp:

#ifndef __SIGNAL_HPP__
#define __SIGNAL_HPP__

#include <algorithm>

template <class t>
class signal {
    public:
    signal<t>(t value, double timestamp);

    signal<t>();

    t get();
    double get_timestamp();

    private:
    t value;
    double timestamp;
};


template <class t>
signal<t>::signal(t value, double timestamp) {
    this->value = value;
    this->timestamp = timestamp;
}

template <class t>
signal<t>::signal() {
    timestamp = -1;
}

template <class t>
double signal<t>::get_timestamp() {
    return timestamp;
}


template <class t>
t signal<t>::get() {
    return value;
}

#endif

src/utils.h:

#ifndef __UTILS_H__
#define __UTILS_H__

#include <cstring>

template <class t>
class ring_buffer {
    public:

    ring_buffer(size_t buffer_size);
    ring_buffer();
    ~ring_buffer();
    void push(t data);
    t peek();
    t pop();

    private:
    t* buffer;
    int data_start_idx;
    int data_end_idx;
    size_t size;
};

template <class t>
ring_buffer<t>::ring_buffer(size_t buffer_size) {
    buffer = new t[buffer_size];
    size = buffer_size;
    data_start_idx = 0;
    data_end_idx = 0;
}

template <class t>
ring_buffer<t>::ring_buffer() {
    ring_buffer(64);
}

template <class t>
ring_buffer<t>::~ring_buffer() {
    delete[] buffer;
}

template <class t>
t ring_buffer<t>::peek() {
    return buffer[data_start_idx];
}

template <class t>
t ring_buffer<t>::pop() {
    t data = buffer[data_start_idx];
    data_start_idx = (data_start_idx + 1) % size;
    return data;
}

template <class t>
void ring_buffer<t>::push(t data) {
    buffer[data_end_idx] = data;
    data_end_idx = (data_end_idx + 1) % size;
}

#endif

src/test/delay_test.cpp:

#include <iostream>
#include <stdio.h>
#include "../signals/signal.hpp"
#include "../signals/filter.hpp"
#include "../signals/delay.hpp"

int main(int argc, char **argv) {
    delay d = delay(0.5, 1024, signal<double>(0, 0));

    double sig = 0;
    double t = 0;
    signal<double> s = signal<double>(0, 0);
    while (true) {
        s = d.apply(signal<double>(sig, t));
        std::cout << "Signal out of delay filter: (" << s.get() << ", " << s.get_timestamp() << ").\n";

        sig += 1;
        t += 0.1;

        getchar();
    }

    return 0;
}
Kyle Rush
  • 153
  • 1
  • 1
  • 8
  • There's not really any timing involved, no threading or anything. I'm simulating the signal so everything runs in discrete timesteps. I know it's definitely being modified **inside** `cout <<` via GDB watchpoint, I'll add the exact location in my post. – Kyle Rush Feb 16 '17 at 19:53
  • 2
    There is nothing obviously wrong. `cout` will not mutate the arguments you pass to it. Perhaps your `get` functions or `operator<<` have side effects? – François Andrieux Feb 16 '17 at 19:56
  • My get function shouldn't, it's just a basic 1-line accessor method (edited in above) I have no idea why it would. I don't know if operator<< would have any though. – Kyle Rush Feb 16 '17 at 20:01
  • I would recommend an [SSCCE](http://sscce.org). – Jason R Feb 16 '17 at 20:02
  • May be an UB (cause by buffer overflow). – felix Feb 16 '17 at 20:04
  • @KyleRush If you didn't override `operator<<` then there should be no side effects. This example doesn't look like it would cause the behavior you describe. We can't test that though, because it doesn't compile. You will definitively need to provide an [MCVE](http://stackoverflow.com/help/mcve). – François Andrieux Feb 16 '17 at 20:06
  • What's the definition of `buffer`? – Mark B Feb 16 '17 at 20:20
  • Please edit your question to contain a [mcve] – Slava Feb 16 '17 at 20:22
  • I've edited my post with an MCVE, hopefully this helps – Kyle Rush Feb 16 '17 at 20:33
  • 2
    You have a lot of issues in your code - new[] must be followed by delete[], violation of rule of 3 etc. – Slava Feb 16 '17 at 20:36
  • I'm aware, this isn't intended to be production grade code, I'm just putting together a quick prototype of an idea I had to validate it to myself. Would those issues cause the behavior of `cout <<` to change though? It works exactly as I'd expect it to if I delete the `cout` statement in delay_test.cpp. – Kyle Rush Feb 16 '17 at 20:39
  • Then you better fix that. First of all why do you avoid member initializer list, to create memory leak right there? – Slava Feb 16 '17 at 20:41
  • peek returns a pointer to uninitialized memory when buffer is empty. You then interpret it. – stark Feb 16 '17 at 20:45
  • @Slava I'm not sure I follow you on the memory leak. Aside from using `delete` where I should have used `delete[]` that buffer is allocated in the constructor and deallocated in the destructor, it shouldn't outlive the ring_buffer instance that's using it. Not sure what a member initialization list would do there either, since it's a dynamically sized memory region. – Kyle Rush Feb 16 '17 at 20:46
  • 2
    @KyleRush are you aware of the Rule of 3/5 and why violating it leads to memory leak and/or UB? – Slava Feb 16 '17 at 20:48
  • @stark The buffer shouldn't be empty when it;s in use though, it's populated fully with the initial condition value during the delay constructor. The whole buffer should contain (0, 0), and I've verified with GDB that it does prior to the call to `cout` – Kyle Rush Feb 16 '17 at 20:49
  • @Slava, I'm aware of it but I don't see how it relates in this case. As far as I can tell the only entities being move/copied should be signal values, which should be trivially moveable/copyable. – Kyle Rush Feb 16 '17 at 20:53
  • @KyleRush how about `ring_buffer` is it copyable? What happens in constructor of delay with statement `buf=...` ? – Slava Feb 16 '17 at 20:55
  • _"If this is too much I can try to cut it down further but I run the risk of eliminating the bug if it gets much more minimal:"_ Yes, that's the point of a MCVE, the act of minimizing it often reveals the bug, because if you remove something and the bug goes away, that something was related to the bug. Nobody wants to read all that code, especially with invalid [reserved names](http://stackoverflow.com/q/228783/981959) like `__DELAY_HPP__`. Stop putting underscores in your include guards, you're not writing standard library code. – Jonathan Wakely Feb 16 '17 at 20:57
  • 1
    The default constructor of `ring_buffer` doesn't initialise anything, it creates an anonymous object which is immediately destroyed. And your lack of assignment operator leads to undefined behaviour after `buf = ring_buffer >(buffer_size);`. – molbdnilo Feb 16 '17 at 20:59
  • @Slava, yep that was it. Looks like I've got to do some familiarizing with the way C++ does things because I didn't even think of that as causing a copy/assignment even though it's clear that it does now. Thanks! – Kyle Rush Feb 16 '17 at 21:34
  • @KyleRush : The proper C++ way would be to avoid manual memory management and use `std::vector` instead of `t*`. Then you don't need to write _any_ special members – the not-coincidentally-named Rule of Zero. – ildjarn Feb 17 '17 at 02:08

1 Answers1

0

When you get weird effects like this it always means some kind of memory corruption, due to some memory management bug.

Running your delay_test under valgrind produces thousands of errors:

==28617== Invalid write of size 8
==28617==    at 0x400EED: ring_buffer<signal<double> >::push(signal<double>) (in /tmp/sig/delay_test)
==28617==    by 0x400C10: delay::delay(double, unsigned long, signal<double>) (in /tmp/sig/delay_test)
==28617==    by 0x401065: main (in /tmp/sig/delay_test)
==28617==  Address 0x5ab40c0 is 0 bytes inside a block of size 16,384 free'd
==28617==    at 0x4C2D6FA: operator delete[](void*) (vg_replace_malloc.c:621)
==28617==    by 0x400E04: ring_buffer<signal<double> >::~ring_buffer() (in /tmp/sig/delay_test)
==28617==    by 0x400BDC: delay::delay(double, unsigned long, signal<double>) (in /tmp/sig/delay_test)
==28617==    by 0x401065: main (in /tmp/sig/delay_test)
==28617==  Block was alloc'd at
==28617==    at 0x4C2C8F9: operator new[](unsigned long) (vg_replace_malloc.c:423)
==28617==    by 0x400E3E: ring_buffer<signal<double> >::ring_buffer(unsigned long) (in /tmp/sig/delay_test)
==28617==    by 0x400BB4: delay::delay(double, unsigned long, signal<double>) (in /tmp/sig/delay_test)
==28617==    by 0x401065: main (in /tmp/sig/delay_test)
==28617== 
...
...
==28617== HEAP SUMMARY:
==28617==     in use at exit: 0 bytes in 0 blocks
==28617==   total heap usage: 5 allocs, 5 frees, 92,160 bytes allocated
==28617== 
==28617== All heap blocks were freed -- no leaks are possible
==28617== 
==28617== For counts of detected and suppressed errors, rerun with: -v
==28617== ERROR SUMMARY: 2060 errors from 8 contexts (suppressed: 0 from 0)

You're reading and writing from memory after it's been deallocated, i.e. the memory locations being changed are not your variables, they are in memory that has been deallocated and then reallocated for something else.

Your ring_buffer manages dynamically allocated memory but doesn't have a user-defined copy constructor or assignment operator. That means it is completely broken.

Jonathan Wakely
  • 166,810
  • 27
  • 341
  • 521
  • Yep, this is it. Defined the necessary copy constructors in ring_buffer and it now functions as expected. Looks like I've got some reading to do on C++'s object instantiation procedures. Thanks! – Kyle Rush Feb 16 '17 at 21:29