List:MySQL++« Previous MessageNext Message »
From:Jonathan Wakely Date:December 16 2007 12:51am
Subject:Exception-safety of Connection::set_option(Option*)
View as plain text  
valgrind reports this memory leak,

==10279== 43 (16 direct, 27 indirect) bytes in 1 blocks are definitely
lost in loss record 3 of 4
==10279==    at 0x4A06205: operator new(unsigned long) (vg_replace_malloc.c:167)
==10279==    by 0x4FB5AA7: mysqlpp::DBDriver::connect(char const*,
char const*, unsigned, char const*, char const*, char const*)
(dbdriver.cpp:83)
==10279==    by 0x4FB2E4E: mysqlpp::Connection::connect(char const*,
char const*, char const*, char const*, unsigned) (connection.cpp:98)
==10279==    by 0x4030D4: main (multiquery.cpp:153)
==10279==
==10279== LEAK SUMMARY:
==10279==    definitely lost: 16 bytes in 1 blocks.
==10279==    indirectly lost: 27 bytes in 1 blocks.
==10279==      possibly lost: 0 bytes in 0 blocks.
==10279==    still reachable: 61,344 bytes in 16 blocks.
==10279==         suppressed: 0 bytes in 0 blocks.

This is caused by passing an option to DBDriver::set_option_default()
and not checking if it was already set, which means the Option object
is leaked.

There is also a potential leak from DBDriver::set_option, if and error
occurs. This therefore affects Connection::set_option as well, e.g.
try setting this option which isn't supported by your version of
mysql:

struct SuperNewOption : Option {
    Option::Error set(DBDriver*) { return Option::err_api_limit; }
};

Currently this is not exception-safe and so leaks memory:

    con.set_option(new SuperNewOption());

It can be fixed with awkward code like:

    std::auto_ptr<Option> p(new SuperNewOption());
    if (set_option(p.get()))
        p.release();

But the library can make it safe just by guaranteeing that
set_option() and set_option_default() will take ownership of the
pointer and either set it successfully (to be released later) or
release it immediately on failure.  This makes exception-safety much
simpler for users calling set_option().

With this patch set_option gives the basic exception-safety guarantee,
so the first example is exception safe.  It fixes the valgrind report
too, of course.

The downside is that the BadOption exception thrown by
Connection::set_option() cannot use the Option pointer, since it has
already been deleted by DBDriver::set_option().  My patch "solves"
this by removing the Option stored in BadOption.  You might want to do
something else there, but it will be difficult unless you weaken the
guarantee on DBDriver::set_option() and code carefully to check return
values.  Is BadOption::what_option() essential?

The patch also changes DBDriver::set_option to call typeid(*o) not
typeid(o) because the  type of 'o' is just Option* so it doesn't tell
you anything useful. typeid(*o) gives you the derived type.

Jon

Attachment: [text/x-patch] mysqlpp-optionleak.patch
Thread
Exception-safety of Connection::set_option(Option*)Jonathan Wakely16 Dec
  • Re: Exception-safety of Connection::set_option(Option*)Warren Young17 Dec