Missing cleanup in destructor of libember::ber::AsyncBerReader

[GoogleCodeExporter]

I think a call to AsyncBerReader::reset() is required to do the whole cleanup 
in libember::ber::AsyncBerReader::AsyncBerReader~().

Original issue reported on code.google.com by nullable...@gmail.com on 20 Jun 2013 at 7:56

Attachments:

• AsyncBerReader.ipp.patch

[GoogleCodeExporter]

You are correct, the current implementation leaks memory when destroyed.

After browsing through the implementation for a bit it appears to me that this 
class could generally use some memory safety lovin', e.g. looking at

void AsyncBerReader::pushContainer()

it's pretty obvious that this method would leak the allocated container in case 
of m_stack.push() throwing an exception.

@Marius: Could you have a look at this? Otherwise you can assign this issue to 
me and I'll fix it in the next few days, after grasping the intended sematics 
of the reader implementation.

Original comment by Kimon.Ho...@lawo.com on 20 Jun 2013 at 10:35

[GoogleCodeExporter]

Yes, I will take a look at this issue and fix it today or tomorrow latest.

Original comment by marius.k...@l-s-b.de on 25 Jun 2013 at 9:08

• Changed state: Accepted

[GoogleCodeExporter]

This issue was closed by revision r31.

Original comment by marius.k...@l-s-b.de on 27 Jun 2013 at 10:46

• Changed state: Fixed

[GoogleCodeExporter]

The destructor now calls the reset() method and AsyncBerReader::pushContainer 
catches any exceptions during the memory allocation and the push operation.

Original comment by marius.k...@l-s-b.de on 27 Jun 2013 at 10:47

[GoogleCodeExporter]

Regarding the changes of r31 inside void AsyncBerReader::pushContainer():

1.) I think translating the exception is not a big benefit and i think 
rethrowing the exception (by just throw;) would be better.
2.) In my opinion, the code would be much more smarter with RAII (something 
like boost::scoped_ptr or std::unique_ptr, maybe simpler).

Are there opinions on that?

Original comment by nullable...@gmail.com on 27 Jun 2013 at 12:53

[GoogleCodeExporter]

I also think a solution based on RAII should (always) be preferred.
But in this case std::auto_ptr<> should be used instead of std::unique_ptr<> or 
boost::scoped_ptr<>, because we base libember solely on C++03 at the meoment.

Original comment by Kimon.Ho...@lawo.com on 27 Jun 2013 at 2:15

[GoogleCodeExporter]

I changed the class to use std::auto_ptr, but it was necessary to remove the 
member pointing to the current container, so some further testing is required 
but it looks good so far. Please take a look at it if possible.

Original comment by marius.k...@l-s-b.de on 28 Jun 2013 at 4:23