SQLite Forum

Vallgrind reports memory leak in libsqlite3.so.0.8.6
Login

Vallgrind reports memory leak in libsqlite3.so.0.8.6

(1) By anonymous on 2021-02-25 04:59:28 [link] [source]

I used Vlagrind to verify that my code was without leaks, and I was surprised by this error (this was the only error Valgrind reported):

Leak_Definitylost 613 (384 direct, 229 bytes in one block definitely lost in los record 4 of 4)
0x4ddd56 sqlite_load_extension
rc = sqlite3_load_extension(DB, "/usr/lib/sqlite3/pcre\0", 0, &ErrMessage);

(2) By Larry Brasfield (larrybr) on 2021-02-25 05:23:25 in reply to 1 [link] [source]

... leaks ... sqlite3_load_extension(DB, "/usr/lib/sqlite3/pcre0", 0, &ErrMessage)

I think this is not a SQLite library problem. The project has a similarly-purposed extension, but as written it could not be loaded as "pcre" because it names itself "regexp". I found this extension which would likely be loaded as "pcre", but it is not part of the SQLite project.

Would you be willing to use this regexp.c instead, and load it as "regexp"? If that leaks, there is an issue fixable within the SQLite project.

(4) By anonymous on 2021-02-25 05:35:30 in reply to 2 [link] [source]

It's not pcre. I checked that. It also occurred with LIBUIC which I also checked. Neither reported a leak.

(7.1) By Larry Brasfield (larrybr) on 2021-02-25 06:18:51 edited from 7.0 in reply to 4 [link] [source]

Are you claiming that you got regexp.c from the SQLite project source, then built it as a dynamically loaded library named "pcre.so" or "pcre.dll"? That is a strange thing to do, and as I said, unless you changed the code, it expects its sqlite3_regexp_init(...) entry point to be called. That is not going to happen with the call to sqlite3_load_extension() that you posted.

Assuming you did manage to load something built from a close derivative of the project's regexp.c, Stephan's point about the error message return is valid. Maybe the module loads and runs without error, making this not the source of the leak you are seeing, but you do need to free that out string if it comes back non-NULL.

It also occurred with LIBUIC which I also checked. Neither reported a leak.

I don't know what to make of that. Neither whats? What also occurred with LIBUIC? And what is LIBUIC?

(9) By anonymous on 2021-02-25 06:24:40 in reply to 7.0 [link] [source]

LIBUIC checks e.g. if it's a valid UIC-number (used by railway companies).
The algorithm is based on the Luhn number algorithm.

There is only one error message. So I disabled loading the extensions. That resulted in three errors with sqlite3_exec. I commented those out and all is well. I can open the sqlite database and close it. Valgrind is happy. Execute a query and the error is back.

It seems to me that there is something amiss with the sqlite3_load_extension, sqlite3_exec, and sqlite3_step. All three report an issue with the malloc() function.                                                                                                 

All other sqlite3 calls are fine!

(10) By anonymous on 2021-02-25 06:49:13 in reply to 7.1 [link] [source]

I found the LIBUIC code. It is source code in the public domain.

Usage: select uic(31816650286);  the checksum is 0 (https://de.wikipedia.org/wiki/UIC-Wagennummer)

/*
 * Implementation to compute the UIC number
 *
*/
static void uic_function(sqlite3_context *ctx, int argc, sqlite3_value **argv){

    assert(argc == 1);

    const unsigned char* cArgument = sqlite3_value_text(argv[0]);

    int digit = 0;
    int checksum = 0;
    const int NumberOfDigits = strlen((const char*) cArgument);

    if (NumberOfDigits == 0) {
        sqlite3_result_error(ctx, "UIC (error): Use UIC(\"123456\")", -1);
        return;
    }

    //int nSum = 0, isSecond = false;
        for (int i = 0; i < NumberOfDigits; i++) {

        /**< Get ASCII value and convert to a number */
        if ((int) cArgument[i] > 57 || (int) cArgument[i] < 48) {
            /**<  Handle the error if it's not a number */
            sqlite3_result_error(ctx, "UIC expects a number", -1);
            return;
        }
        else {
            digit = (int) cArgument[i] - 48;
        }

        if (i % 2 == 0) {
            digit *= 2;
        }

        // We add two digits to handle
        // cases that make two digits after
        // doubling
        if (digit > 9) {
            checksum += digit / 10;
            checksum += digit % 10;
        }
        else {
            checksum += digit;
        }
    }

    checksum = 10 - checksum % 10;

    sqlite3_result_int(ctx, (checksum == 10 ? 0 : checksum));
}

/** \brief
 *
 * \param *db pointer to the database
 * \param pzErrMsg a pointer to a pointer for the error message (null terminated?)
 * \param pApp a point to the SQLite3 API
 * \return SQLITE_OK or an error
 *
 */
int sqlite3_extension_init(sqlite3* db, char **pzErrMsg, const sqlite3_api_routines *pApi){

    int rc = SQLITE_OK;

    SQLITE_EXTENSION_INIT2(pApi);

    rc = sqlite3_create_function(db, "uic", 1, SQLITE_TEXT, 0, uic_function, 0, 0);

  return rc;
}

(3) By Stephan Beal (stephan) on 2021-02-25 05:29:08 in reply to 1 [link] [source]

rc = sqlite3_load_extension(DB, "/usr/lib/sqlite3/pcre0", 0, &ErrMessage);

It looks suspiciously like you are failing to free the error message (assuming you are getting one). If you intend to ignore any error message, pass NULL as the final argument.

https://sqlite.org/c3ref/load_extension.html

PS: there is no benefit to adding slash-zero to the end of string literals. They already have an implicit NUL terminator byte.

(5) By anonymous on 2021-02-25 05:37:51 in reply to 3 [link] [source]

The error message is used.

    const char* get_err_msg(void) { return ErrMessage; }

(6) By Stephan Beal (stephan) on 2021-02-25 05:48:43 in reply to 5 [link] [source]

The error message is used.

Then that looks like the source of your leak. sqlite3_load_extension()'s docs explain that the error message is allocated dynamically and must be freed by the caller.

sqlite3 is literally one of the best-tested pieces of software in the world, and claims that it is leaking memory require proof that it's doing so. Your valgrind trace shows a place where it's clearly possible (even likely) that the leak is caused by ignoring the instructions of the sqlite3_load_extension() docs.

Here's a way to quickly test that hypothesis: pass NULL as the final argument, instead of &ErrMessage, and run it through valgrind again. My bet is that the leak goes away.

(8) By anonymous on 2021-02-25 06:05:43 in reply to 6 [link] [source]

I changed it to NULL and the error remains.

The destructor cleans up the Errmessage and yes, the error message variable is a private variable (char* ErrMessage) of this class thus passed by reference to SQLite. Valgrind isn't complaining about that.

PCRE contains the following:
/* Free and reclaim all the memory used by a previously compiled
** regular expression.  Applications should invoke this routine once
** for every call to re_compile() to avoid memory leaks.
*/
static void re_free(ReCompiled *pRe){
  if( pRe ){
    sqlite3_free(pRe->aOp);
    sqlite3_free(pRe->aArg);
    sqlite3_free(pRe);
  }
}

Seems to me that this code does clean up its mess.

(11) By Larry Brasfield (larrybr) on 2021-02-25 06:57:09 in reply to 8 [link] [source]

That routine frees resources when called with a pointer to them. That does not imply that the routine is called for every RE compilation, but it looks like that is arranged elsewhere in regexp.c .

Reading regexp.c, it appears to me that it keeps the compiled RE resources across calls to the SQL function that (likely) uses it at each sqlite3_step() call. It also keeps a destructor-equivalent pointer with which to eventually call the very same re_free() you list above. "Eventually" would be when sqlite3_finalize() is called. I imagine you are calling that before your program exits and gives Valgrind a chance to moan about leaks. (Otherwise, you would be mentioning more leaks.)

My take on this is that a serious effort was made to not leak memory in this extension code (assuming it is regexp.c taken from the SQLite source tree, a proposition not yet confirmed or denied by Only Person who knows.) And my reading of the code that resulted from that effort makes me think it should not leak, independently of my confidence in its author.

If you can provide a small program which demonstrates this alleged leak, without dependencies other than upon the SQLite library, I will take a look at it. (I have a leak-instrumented build setup for another library client which would be easily adapted to this.) I presume that any use of the RE-matching SQL function will suffice to instigate the leak with your code.

If regexp.c does leak when used properly, that is worth knowing and fixing. However, I doubt that you have found a leak in extension loading and unloading, or the rest of the SQLite library. I expect to read your small program and spot why it leaks, then fix it and show an absence of leaks.

(13) By Keith Medcalf (kmedcalf) on 2021-02-25 07:40:48 in reply to 8 [link] [source]

The PCRE extension is designed to leak memory. I would suggest you take a look at the code for the extension which allocates memory off the heap and never releases it.

Of course, you would have to look at the code that you happen to be using for the PCRE extension, which mayhaps is or mayhaps is not the same as I found with the Google which very obviously allocates heap memory that is never released.

It did not take much examination to determine this fact beyond a very cursory look at the init function.

(14) By anonymous on 2021-02-25 09:40:37 in reply to 13 [link] [source]

So I downloaded PCRE.
Compiled it in debug mode and with SQLite v3.35 as the host. Then I ran Valgrind.
No memory leaks.

Although, there is one funny statement:

    /*
    ** Invoke this routine to register the regexp() function with the
    ** SQLite database connection.
    */
    #ifdef _WIN32
    __declspec(dllexport)
    #endif
    int sqlite3_regexp_init(
      sqlite3 *db,
      char **pzErrMsg,
      const sqlite3_api_routines *pApi
    ){
      int rc = SQLITE_OK;
      SQLITE_EXTENSION_INIT2(pApi);
      rc = sqlite3_create_function(db, "regexp", 2, SQLITE_UTF8, 0, re_sql_func, 0, 0);
      /* -> disabled  funny statement
      rc = sqlite3_create_function(db, "regexp", 2, SQLITE_UTF8|SQLITE_INNOCUOUS,
                                   0, re_sql_func, 0, 0);
      */
      return rc;
    }

So I wrote a small C++ program - nothing fancy -:
#include <iostream>
#include "sqlite3.h"

class DBase
{
    public:
        DBase(const std::string& dbName)
        {
            rc = sqlite3_open(dbName.c_str(), &DB);
            //(dbName.c_str(), &DB);
        }

        ~DBase() { sqlite3_close(DB); }

        void execute(const std::string& );

        // get the result from SQLite
        static int exec_sql(void *unused, int rows, char **data, char **column)
        {
                for (int i = 0; i < rows; i++) {
                    if (i == 1) {
                        std::cout << data[i];
                    }
                    else {
                        std::cout << column[i] << " -> "<< data[i];
                    }
                    std::cout  << ", ";
            }

            std::cout << std::endl;

            return  SQLITE_OK;
        }

        const int getResult(void) { return rc; }
        const char* get_err_msg(void) { return ErrMessage; }

    private:
        sqlite3* DB;
        int rc = -1;
        char* ErrMessage;
};



void DBase::execute(const std::string& command) {

    if (command.length() == 0) {
        rc = SQLITE_ERROR;
    }

    rc = sqlite3_exec(DB, command.c_str(), exec_sql, 0, &ErrMessage);

};

int main()
{
    DBase db("/Project/OMT DB development/omt.db");

    db.execute("SELECT LOC_UIC, UIC(LOC_UIC), LOC_TOWN FROM LOCATION WHERE LOC_UIC REGEXP '8[0-9]{7}' AND UIC(LOC_UIC) != 3;");


    return 0;
}

The problems begin when I want to get the error message

(15) By Keith Medcalf (kmedcalf) on 2021-02-25 09:54:49 in reply to 14 [source]

Why would you call that PCRE?

(16) By anonymous on 2021-02-25 10:06:39 in reply to 15 [link] [source]

Sorry. I compiled the complete PCRE code that can be found on this location https://sqlite.org/src/file?name=ext/misc/regexp.c&ci=trunk

The link was given to me in this thread. So to be clear:
I've tested the compiled PCRE code and all is fine.
I've tested the PCRE code I compiled and all is fine.

And in the example code I wrote got the error message: No such function: UIC.

(18) By ddevienne on 2021-02-25 10:30:46 in reply to 14 [link] [source]

From the exec doc:

To avoid memory leaks, the application should invoke sqlite3_free()
on error message strings returned through the 5th parameter of sqlite3_exec()

So

~DBase() { sqlite3_close(DB); }

should be

class DBase { ...
    char* ErrMessage = nullptr;
};

DBase::~DBase() {
    sqlite3_free(ErrMessage);
    sqlite3_close(DB);
}

void DBase::execute(const std::string& command) {
    sqlite3_free(ErrMessage);
    ErrMessage = nullptr;
    ...
}

I mean, as a start. That's not quite modern C++ :), no offence meant.
I assume above the _free is a no-op for nulls, I kinda recall it is.

(19) By anonymous on 2021-02-25 11:06:34 in reply to 18 [link] [source]

Yes, it is old school C++ to avoid a discussion what might or might not be supported. C++v11 and for the application I am developing it's C++v20.

sqlite3_free(ErrMessage) will result in a core dump because DBase is the owner of the memory allocated to ErrMessage.

In the actual code ErrMsg = nullptr;

So lets try your suggestion:

#include <iostream>
#include "sqlite3.h"

class DBase
{
    public:
        DBase(const std::string& dbName)
        {
            rc = sqlite3_open(dbName.c_str(), &DB);

        }

        ~DBase()
        {
            sqlite3_free(ErrMessage);
            sqlite3_close(DB);
        }

        void execute(const std::string& );

        // get the result from SQLite
        static int exec_sql(void *unused, int rows, char **data, char **column)
        {
                for (int i = 0; i < rows; i++) {
                    if (i == 1) {
                        std::cout << data[i];
                    }
                    else {
                        std::cout << column[i] << " -> "<< data[i];
                    }
                    std::cout  << ", ";
            }

            std::cout << std::endl;

            return  SQLITE_OK;
        }


        const int getResult(void) { return rc; }
        const char* get_errMSG(void) { return ErrMessage; }

    private:
        sqlite3* DB;
        int rc = -1;
        char* ErrMessage = nullptr;
};



void DBase::execute(const std::string& command) {

    if (command.length() == 0) {
        rc = SQLITE_ERROR;

    }
    rc = sqlite3_exec(DB, command.c_str(), exec_sql, 0, &ErrMessage);

    sqlite3_free(&ErrMessage);

};


int main()
{
    DBase db("/Project/OMT DB development/omt.db");

    db.execute("SELECT LOC_UIC, UIC(LOC_UIC), LOC_TOWN FROM LOCATION WHERE LOC_UIC REGEXP '8[0-9]{7}' AND UIC(LOC_UIC) != 3;");

    std::cout << db.get_errMSG() << std::endl;


    return 0;
}

Which goes horribly wrong and Valgri8nd screams: invalid free/ delete / delete() / realloc() and that is what I expected to happen

(20) By Stephan Beal (stephan) on 2021-02-25 11:40:03 in reply to 19 [link] [source]

sqlite3_free(&ErrMessage);

Remove the ampersand. (Pardon brevity - on a train.)

(21) By anonymous on 2021-02-25 12:07:42 in reply to 20 [link] [source]

No worries. I've overlooked that erroneous sqlite_free() line. Now it only appears in the destructor.Compiled the source with no errors and warnings. Valgrind is also happy.

So lets load the extensions:
#include <iostream>
#include "sqlite3.h"

class DBase
{
    public:
        DBase(const std::string& dbName)
        {
            rc = sqlite3_open(dbName.c_str(), &DB);
            if (rc == SQLITE_OK)
            {
                // Load the extensions
                rc = sqlite3_load_extension(DB, "/usr/lib/sqlite3/libuic",  0, NULL);
                rc = sqlite3_load_extension(DB, "/usr/lib/sqlite3/pcre", 0, NULL);
            }
        }

        ~DBase()
        {
            sqlite3_free(ErrMessage);
            sqlite3_close(DB);
        }

        void execute(const std::string& );

        // get the result from SQLite
        static int exec_sql(void *unused, int rows, char **data, char **column)
        {
                for (int i = 0; i < rows; i++) {
                    if (i == 1) {
                        std::cout << data[i];
                    }
                    else {
                        std::cout << column[i] << " -> "<< data[i];
                    }
                    std::cout  << ", ";
            }

            std::cout << std::endl;

            return  SQLITE_OK;
        }


        const int getResult(void) { return rc; }
        const char* get_errMSG(void) { return ErrMessage; }

    private:
        sqlite3* DB;
        int rc = -1;
        char* ErrMessage = nullptr;
};



void DBase::execute(const std::string& command) {

    if (command.length() == 0) {
        rc = SQLITE_ERROR;

    }
    rc = sqlite3_exec(DB, command.c_str(), exec_sql, 0, &ErrMessage);

};

int main()
{
    DBase db("/Project/OMT DB development/omt.db");

    db.execute("SELECT LOC_UIC, UIC(LOC_UIC), LOC_TOWN FROM LOCATION WHERE LOC_UIC REGEXP '8[0-9]{7}' AND UIC(LOC_UIC) != 3;");

    std::cout << db.get_errMSG() << std::endl;


    return 0;
}

And the memory leak error is back.

(22) By anonymous on 2021-02-25 12:35:23 in reply to 20 [link] [source]

From the SQLite documentation:
Here https://sqlite.org/c3ref/funclist.html
and here https://sqlite.org/c3ref/free.html

It states: If X is a memory allocation previously obtained from sqlite3_malloc(), sqlite3_malloc64(), sqlite3_realloc(), or sqlite3_realloc64(), then sqlite3_msize(X) returns the size of that memory allocation in bytes. The value returned by sqlite3_msize(X) might be larger than the number of bytes requested when X was allocated. If X is a NULL pointer then sqlite3_msize(X) returns zero. If X points to something that is not the beginning of memory allocation, or if it points to a formerly valid memory allocation that has now been freed, then the behavior of sqlite3_msize(X) is undefined and possibly harmful.

The memory returned by sqlite3_malloc(), sqlite3_realloc(), sqlite3_malloc64(), and sqlite3_realloc64() is always aligned to at least an 8 byte boundary, or to a 4 byte boundary if the SQLITE_4_BYTE_ALIGNED_MALLOC compile-time option is used.

The pointer arguments to sqlite3_free() and sqlite3_realloc() must be either NULL or else pointers obtained from a prior invocation of sqlite3_malloc() or sqlite3_realloc() that have not yet been released.

The application must not read or write any part of a block of memory after it has been released using sqlite3_free() or sqlite3_realloc(). 

So why is there a need to free memory not owned by SQlite? The is sqlite3_malloc64() statement in the code.

(12) By anonymous on 2021-02-25 06:57:27 in reply to 6 [link] [source]

I agree. SQLite has proven itself as an excellent tool that is reliable.
I've been using it for analyzing data and testing for a decade, and it never failed.

Privately, it is also the DB of choice to keep track of all sorts of things.

This is not a sponsored statement, and I am not affiliated with the SQLite organization.

(17) By Ryan Smith (cuz) on 2021-02-25 10:15:29 in reply to 12 [link] [source]

This is not a sponsored statement, and I am not affiliated with the SQLite organization.

Do not fear, nobody thought that.

The day Dr. Hipp starts needing to go around paying people to say how cool SQLite is, is the day the World ends. :)