-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Use temporary memory database connection to quote the password #36956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Use temporary memory database connection to quote the password
|
Could you add some testing? |
In principle, a test is already in place: efcore/test/Microsoft.Data.Sqlite.Tests/SqliteConnectionTest.cs Lines 264 to 276 in aeb51fa
You just need to enable either Without the fix the test will fail for SQLite 3.48.0 or later with an error message ( |
| using (var inMemoryConnection = new SqliteConnection("Filename=:memory:")) | ||
| { | ||
| inMemoryConnection.Open(); | ||
| quotedPassword = ExecuteScalar( |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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:")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@dotnet-policy-service agree |
bricelam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@bricelam: Thanks. Nevertheless, I have reconsidered the implementation. I have doubts as to whether the function 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? |
|
@utelle I agree, it's not needed here. Proposed code looks good to me. |
Fixes #35760