Sqlite_orm: storage.backup_to(filename) crashes in backup_tests.cpp

[juandent]

TEST_CASE("backup") in backup_tests.cpp calls retain (see below) with this == nullptr.

First it calls:

TEST_CASE("backup") {
    using namespace BackupTests;
    using Catch::Matchers::UnorderedEquals;
    const std::string usersTableName = "users";
    auto makeStorage = [&usersTableName](const std::string &filename) {
        return make_storage(
            filename,
            make_table(usersTableName, make_column("id", &User::id, primary_key()), make_column("name", &User::name)));
    };
    const std::string backupFilename = "backup.sqlite";
    SECTION("to") {
        ::remove(backupFilename.c_str());
        auto storage2 = makeStorage(backupFilename);
        auto storage = makeStorage("");
        storage.sync_schema();
        storage.replace(User{1, "Sharon"});
        storage.replace(User{2, "Maitre"});
        storage.replace(User{3, "Rita"});
        REQUIRE(!storage2.table_exists(usersTableName));
        SECTION("filename") {
            storage.backup_to(backupFilename);       // THIS CRASHES
        }

namespace sqlite_orm {

    namespace internal {

        struct connection_holder {

            connection_holder(std::string filename_) : filename(move(filename_)) {}

            void retain() {
                ++this->_retain_count;      // this == 000000000

What is happening?

Regards,
Juan

[juandent]

The cultrip is found in sqlite_orm.h method backup_t make_backup_to(const std::string &filename) of class sqlite_orm::internal::storage_base

``` backup_t make_backup_to(const std::string &filename) {
auto holder = std::make_unique(filename);

.return {connection_ref{*holder}, "main", this->get_connection(), "main", move(holder)};
```

The correct solutions are:

1. auto conn_ref{ connection_ref{*holder} };
   return { conn_ref, "main", this->get_connection(), "main", move(holder) };

2. or change the order of the none-static data elements for the class struct backup_t

This solves this issue and I bet some others!!

[fnc12]

@juandent what branch? master or dev?

[juandent]

master

[juandent]

the problem is that move(holder) is done before connection_ref{*holder}, thus passing a nullptr to connection_ref

[juandent]

this happens because the order of the data members in the class backup_t moves holder first making it empty...

```
std::unique_ptr holder;
connection_ref to;
connection_ref from;
'''

[fnc12]

@juandent ok thank you. I cannot reproduce it. But I think the reason is that the order of expression evaluation is not determined before C++17. Anyway I shall apply your fix and merge it soon

[fnc12]

The fix is here https://github.com/fnc12/sqlite_orm/pull/542

[fnc12]

@juandent please help me. Some builds failed https://travis-ci.org/github/fnc12/sqlite_orm/jobs/704755026 and I cannot reproduce it with Xcode. Looks like exact error you showed here before. Also it turned out that backup_tests.cpp were not included into cmakelists file and this is why these tests failed - they were not supported actually (my bad). My PR uses the fix you suggested bug it calls SEGFAULT and I need to fix it. I shall appreciate it much

[juandent]

although the order of operations in an expression was (before C++ 17) unpredictable, the order of initialization of the non-static data members of a class/struct is and has always been deterministic: they are initialized in the exact order they appear in the class -- however not the order they appear in the constructors because they could have different orders. Thus backup_t should declare its non-static data members in the following order to avoid passing an empty holder:

         protected:
            sqlite3_backup *handle = nullptr;
            connection_ref to;
            connection_ref from;
            std::unique_ptr<connection_holder> holder;

Holder must be the last!!

Tell me if this helps you - it should!!

Regards,
Juan

[juandent]

internal::storage_base::make_backup_to should have this code:

            backup_t make_backup_to(const std::string &filename) {
                auto holder = std::make_unique<connection_holder>(filename);
                auto conn_ref{ connection_ref{*holder} };
                return { conn_ref, "main", this->get_connection(), "main", move(holder) };

[juandent]

sorry I was away from my computer - did this help any?

[fnc12]

Thank you for your suggestions. I applied them a minute ago and now we need to wait until unit tests are finished on travis and appveyor. I can take up to 2 hours.

[juandent]

how are you doing ? what happened with the builds?

[fnc12]

same thing. Windows - https://ci.appveyor.com/project/fnc12/sqlite-orm/builds/33905319/job/pa4q8wk57q8kv22e

[fnc12]

@juandent what OS and compiler do you use?

[juandent]

I use Windows 10 Pro and Visual Studio 2019 Version 16.6.3

And I compile with C++ 17 support which is very good in VS

Are you still having problems? Can I help you?

How can I help?

Regards,

Juan Dent

From: Yevgeniy Zakharov [e-mail]
Sent: Saturday, July 4, 2020 12:57 PM
To: fnc12/sqlite_orm [e-mail]
Cc: juandent [e-mail]; Mention [e-mail]
Subject: Re: [fnc12/sqlite_orm] storage.backup_to(filename) crashes in backup_tests.cpp (#540)

@juandent https://github.com/juandent what OS and compiler do you use?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/fnc12/sqlite_orm/issues/540#issuecomment-653799851 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ABI6OFN2TSOHWMWDD4ZRGMDRZ53PFANCNFSM4OPLNGAQ . https://github.com/notifications/beacon/ABI6OFL4SPPWA2DZ6RQUTATRZ53PFA5CNFSM4OPLNGA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOE34DDKY.gif

[juandent]

why don't you install a VMWARE virtual system with Windows and install the (free ) Visual Studio 2019?

[fnc12]

Good news: I am able to reproduce it with VS on Win10 (on virtual machine).

[fnc12]

Ok I fixed windows but mac fails tests. SEGFAULT doesn't happen now.

[juandent]

which requiere is failing?

[fnc12]

https://travis-ci.org/github/fnc12/sqlite_orm/jobs/705096749
https://travis-ci.org/github/fnc12/sqlite_orm/jobs/705096750
https://travis-ci.org/github/fnc12/sqlite_orm/jobs/705096750
Looks like that the reason is different not tests

[fnc12]

Ok I merge the PR cause the issue is not related to this PR

[fnc12]

merged. Please check out dev branch

[fnc12]

is the issue actual?

[juandent]

How do I download the dev branch? I am not too proficient with git yet...
and after downloading how do I run CMAKE on the download to get the binary builds (e.g. examples and tests)?

When I run CMAKE I get this error:

CMake Error at tests/CMakeLists.txt:21 (find_package):
By not providing "FindCatch2.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "Catch2", but
CMake did not find one.

Could not find a package configuration file provided by "Catch2" with any
of the following names:

Catch2Config.cmake
catch2-config.cmake

Add the installation prefix of "Catch2" to CMAKE_PREFIX_PATH or set
"Catch2_DIR" to a directory containing one of the above files. If "Catch2"
provides a separate development package or SDK, be sure it has been
installed.

and I downloade Catch2 but it does not come with Catch2Config.cmake (instead it comes with Catch2Config.cmake.in)

So I am stuck.
Please help me so I can help you!!

Regards,
Juan

[fnc12]

you need to install catch2. It is a bug https://github.com/fnc12/sqlite_orm/issues/535

[fnc12]

closing this issue cause it is fixed. @juandent is you have issues with git please contact me with e-mail. Thanks