Resetting KalmanFilter is broken

[toeklk]

migrated from Bugzilla #798 status NEW severity normal in component core for --- Reported in version trunk on platform All Assigned to: BFL mailinglist

Original attachment names and IDs:

• KalmanFilterReset.diff (ID 619)

On 2010-12-03 15:41:06 +0100, Johannes Meyer wrote:

  Created attachment 619 Overwritten KalmanFilter::Reset() method The current implementation of the method Filter::Reset() is broken, at least for instances of KalmanFilter and subclasses: It overwrites the pointer _post to the posterior density, which is instantiated individually in the subclasses. The subclass will therefore overwrite the prior given by the user after the reset and destruction of the filter leads to undefined behevior. I attached a patch for the KalmanFilter class, but other filters might be affected as well. Best regards Johannes

[kam3k]

Couldn't this bug be resolved by changing the body of Reset() from

_prior = prior;
_post = prior;

to

_prior = prior;
_post = new Gaussian(*prior);

like what is done in the constructor of KalmanFilter?

[toeklk]

Hi Marc,

(my C++ skills are a bit rusty, so please correct me if I got it wrong)

Thx for your input! However, your suggestion leads to a memory leak (the old _post is not freed). You would first need to perfom a delete operation. Also, even if you would do so, this requires memory allocation and hence will break real-time behaviour if you would execute the .reset() operation in a real-time thread.

Johannes' patch (see original comment of the migrate bug) is definitely an alternative, but as he points out, it's not complete: the bug needs to be tackled in all subclasses. Also, I would need to figure out how real-time operating systems deal with the dynamic_cast...

[kam3k]

How is the old _post freed in the current implementation? And sorry, I wasn't aware of the real-time constraints on the design.

[toeklk]

First of all, no need for apologies ;-) (orocos stands for open real-time control software)

Currently, _post is freed in the destructor, but that's one reason why this bug exist. If a reset is performed on a filter,

_post = prior;

and the memory block originally pointed to by _post is lost

[kam3k]

Ah right. That definitely causes a problem. After calling Reset(), _post and _prior will point to the same block of memory. However, the memory pointed to by _prior is owned by the user outside of the filter class. That means, when _post is deleted in the destructor, it also deletes the memory pointed to by _prior. So then the user deletes _prior themselves, it will lead to a double-free.

[kunaltyagi]

Using smart pointers might be an answer. But that might requiring compulsory use of either Boost or a recent C++ compiler. If C++14 is used, using std::unique_ptr<T> and std::shared_ptr<T> would remove the ambiguity of ownership.

Another option is always creating new pointers and copying data from the user-owned pointers. This requires ensuring the user knows their pointers do not affect the BFL data.

As for Dynamic Cast, you can eliminate that making CovarianceSet and ExpectedValueSet pure virtual in Pdf<T>. That'll make the Pdf<T>* automatically get the correct function. Technically, lots of functions should be pure virtual in Pdf. I can send a PR if you'd like that.

[kam3k]

@kunaltyagi Note that std::unique_ptr<T> and std::shared_ptr<T> require only C++11, not C++14. Also, I definitely support the use of smart pointers here, but it would be quite an intrusive overhaul if they were to be used everywhere.

[toeklk]

AFAICS, the RTT uses boost::intrusive_ptr ( see http://www.orocos.org/stable/documentation/rtt/v2.x/api/html/classRTT_1_1base_1_1DataSourceBase.html#af764490461a162f4a62c63770535fb7b ), probably due to its real-time suitedness (but I didn't check it). Are you guys using BFL as standalone, or together with other orocos libraries?

[kunaltyagi]

@kam3k Oops, my bad. I thought C++11 had auto_ptr, not unique_ptr (Spoilers: It had both). Thanks for catching that one.

I'm using BFL standalone, but there must be people using it with the wider orocos lib suite.

[kam3k]

@toeklk I'm using BFL standalone as well.