Bug#954973: [Aptitude-devel] Bug#954973: potential memory leak: forgetting to free error message of libsqlite3 API 'sqlite3_exec'

[David Kalnischkies]

Hi,

(beware, I am not an aptitude developer)

On Thu, Mar 26, 2020 at 10:11:51AM +0800, 李蕊诗 wrote:
> 208:   if(result != SQLITE_OK || msg != NULL)
> 209:  throw exception(errmsg, result);
> 210:+ if(msg)
> 211:+ sqlite3_free(msg)

Wouldn't that be a memory leak every time the if-block is entered as we
will leave the function via 'throw'? Which would be all the time there
is something to clean up as we enter the if if there is a message…


Note through that aptitude uses a typical C++ idiom to work with this
C-based API requiring explicit calls to a free method:
The line before the sqlite3_exec call initializes a struct with the
aptly name "free_on_destroy". It is defined just a few lines further
above and what it does is keeping a reference to "msg" variable on
construction and calling sqlite3_free on it if deconstructed.

Deconstruction of the variable will happen automatically whenever it
leaves scope: Which is true for the 'throw' call as well as for line
210: "}"

Some C++ developers hence if asked for their favorite feature answer
with a cryptic "}". The idiom is called "Resource acquisition is
initialization" (RAII) and super useful in all sorts of ways.


I hence think there is no memory leak, as the freeing is done as
required by the API, but I will leave it to the aptitude devs to confirm
my suspicion & closing it. Thanks for your report & proposing a patch
none the less!


Best regards

David Kalnischkies


signature.asc
Description: PGP signature

Bug#954973: potential memory leak: forgetting to free error message of libsqlite3 API 'sqlite3_exec'

[李蕊诗]

Package: aptitude

Version: 0.8.12

Source: aptitude

I downloaded the newest source code
from:http://deb.debian.org/debian/pool/main/a/aptitude/aptitude_0.8.12.orig.tar.xz.
The bug lies in src/generic/util/sqlite.cc:201-202:

201:  int result = sqlite3_exec(handle, sql.c_str(),
202:callback, data, &msg);

According to libsqlite3 API document of 'sqlite3_exec': "To avoid
memory leaks, the application should invoke sqlite3_free() on error
message strings returned through the 5th parameter of sqlite3_exec()
after the error message string is no longer needed."

So this is potential memory leak, and the patch should be:

208:   if(result != SQLITE_OK || msg != NULL)
209:throw exception(errmsg, result);
210:+   if(msg)
211:+   sqlite3_free(msg)