iwongu / sqlite3pp

SQLite3++ - C++ wrapper of SQLite3 API
MIT License
609 stars 177 forks source link

Making query::rows::get(int,std::string) safe/useful. #75

Closed stewbond closed 1 year ago

stewbond commented 1 year ago

This PR addresses issue #67

The following mundane code is surprisingly unsafe:

sqlite3pp::query query(db, "SELECT ...");
for (auto row : query) {
    for (int col = 0; col < query.column_count(); ++col) {
        std::cout << row.get<std::string>(col) << std::endl;
    }
}

When we row.get<std::string>(col) we expect to get a string. However, if the result contains any NULL cells we will get an undefined object and there is very little a user can do about it. The library may SIGSEGV before a user can detect it. This is because the implementation simply performs a naive cast from char const* to std::string. If the cell is NULL, then it will effectively return std::string(nullptr) which is undefined behaviour.

A work-around that could be done from the user's side is:

sqlite3pp::query query(db, "SELECT ...");
for (auto row : query)
{
    for (int col = 0; col < query.column_count(); ++col)
    {
        auto c = row.get<char const*>(col);
        std::cout << c ? std::string(c) : std::string() << std::endl;
    }
}

But this is really just a way to avoid the unsafe query::rows::get(int idx, std::string). We could use row.get<char const*>(col) to check for null, then do the row.get<std::string>(), but in that case, we should just do the char->string cast ourselves and avoid a potentially expensive call.

The solution (suggested here) is to put that work-around in the library itself, making the function useful again.

The drawback is that the user can't use this function to differentiate between empty and null, but they couldn't use this function for that purpose before either. However, I don't think this is too bad.

The underlying library (libsqlite3.so) will not crash like this if sqlite3_column_*() is called on a NULL cell. While sqlite3_column_text() returns a null pointer, this is meer convenience because it is possible. sqlite3_column_double() returns 0.0 and sqlite3_column_int() returns 0. Re-using this philosophy, returning a default-constructed object is a reasonable response to NULL data, and certainly more reasonable than returning an undefined object which may SIGSEGV before the user can do anything with it.