Skip to content

Conversation

@utelle
Copy link
Contributor

@utelle utelle commented Oct 12, 2025

Fixes #35760

Use temporary memory database connection to quote the password
@ErikEJ
Copy link
Contributor

ErikEJ commented Oct 12, 2025

Could you add some testing?

@utelle
Copy link
Contributor Author

utelle commented Oct 12, 2025

Could you add some testing?

In principle, a test is already in place:

[Fact]
public void Open_works_when_password()
{
#if E_SQLITE3 || WINSQLITE3
Open_works_when_password_unsupported();
#elif E_SQLCIPHER || E_SQLITE3MC || SQLCIPHER
Open_works_when_password_supported();
#elif SQLITE3
Open_works_when_password_might_be_supported();
#else
#error Unexpected native library
#endif
}

You just need to enable either E_SQLCIPHER or E_SQLITE3MC. Of course, you need to make sure that a SQLite version greater or equal 3.48.0 is used under the hood.

Without the fix the test will fail for SQLite 3.48.0 or later with an error message (SQLITE_NOTADB), while it will succeed with the fix.

using (var inMemoryConnection = new SqliteConnection("Filename=:memory:"))
{
inMemoryConnection.Open();
quotedPassword = ExecuteScalar(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExecuteScalar will not use inMemoryConnection here. (It uses _db)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch... that will have to be changed, of course.

var quotedPassword = ExecuteScalar(
"SELECT quote($password);",
var quotedPassword = string.Empty;
using (var inMemoryConnection = new SqliteConnection("Filename=:memory:"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use the ADO.NET interfaces (e.g. SqliteConnection) here. Use the lower-level SQLitePCLRaw APIs (e.g. sqlite3_open_v2) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to adjust the code accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bricelam I tried to adjust the code according to your comments. Please take a look.

@utelle
Copy link
Contributor Author

utelle commented Oct 13, 2025

@dotnet-policy-service agree

@SamMonoRT SamMonoRT requested a review from cincuranet October 13, 2025 14:41
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@utelle
Copy link
Contributor Author

utelle commented Oct 13, 2025

LGTM

@bricelam: Thanks. Nevertheless, I have reconsidered the implementation. I have doubts as to whether the function RetryWhileBusy() should/needs really be used here. I took it from the implementation of ExecScalar(). However, in case of an in-memory database there is no concurrency at all, because only the connection that opened the in-memory database can access it - AFAIK. That is, no way for the connection to be busy under the given circumstances.

Therefore the current implementation

    private string QuotePassword(string pswd, int timeout)
    {
        sqlite3 dbMem;
        var rc = sqlite3_open(":memory:", out dbMem);
        SqliteException.ThrowExceptionForRC(rc, dbMem);

        var timer = Stopwatch.StartNew();
        sqlite3_stmt stmt = null!;
        RetryWhileBusy(() => sqlite3_prepare_v2(dbMem, "SELECT quote($password);", out stmt), timeout, timer);
        try
        {
            sqlite3_bind_text(stmt, 1, pswd);
            RetryWhileBusy(() => sqlite3_step(stmt), () => sqlite3_reset(stmt), timeout, timer);
            return sqlite3_column_text(stmt, 0).utf8_to_string();
        }
        finally
        {
            stmt.Dispose();
            dbMem.Dispose();
        }
    }

could possibly be simplified to the following:

    private string QuotePassword(string pswd)
    {
        sqlite3 dbMem;
        int rc = sqlite3_open(":memory:", out dbMem);
        SqliteException.ThrowExceptionForRC(rc, dbMem);
        sqlite3_stmt stmt = null!;
        try
        {
            rc = sqlite3_prepare_v2(dbMem, "SELECT quote($password);", out stmt);
            SqliteException.ThrowExceptionForRC(rc, dbMem);
            sqlite3_bind_text(stmt, 1, pswd);
            rc = sqlite3_step(stmt);
            SqliteException.ThrowExceptionForRC(rc, dbMem);
            return sqlite3_column_text(stmt, 0).utf8_to_string();
        }
        finally
        {
            stmt.Dispose();
            dbMem.Dispose();
        }
    }

What do you think?

@bricelam
Copy link
Contributor

@utelle I agree, it's not needed here. Proposed code looks good to me.

@cincuranet cincuranet changed the title Fix #35760 Use temporary memory database connection to quote the password Oct 14, 2025
@cincuranet cincuranet merged commit 2e211e4 into dotnet:main Oct 14, 2025
7 checks passed
cincuranet added a commit to cincuranet/efcore that referenced this pull request Oct 14, 2025
@cincuranet
Copy link
Contributor

Thanks.

I did quick cleanup in #36961. And backport to 10 (#36962).

@utelle
Copy link
Contributor Author

utelle commented Oct 14, 2025

I did quick cleanup in #36961. And backport to 10 (#36962).

Thank you very much for cleaning up the code, and for backporting to version 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting the password for an encrypted database fails, if version 3.48.0 or higher of the underlying SQLite library is used

4 participants