New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 894884 link

Starred by 2 users
Status: Started
Owner:
pwnall@chromium.org
Cc:
dewittj@chromium.org
█ harringtond@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Second call to Database::GetCachedStatement DCHECKs if SQL statement has leading spaces

Project Member Reported by carlosk@chromium.org, Oct 12 
When Database::GetCachedStatement is called a second time for the same statement it will DCHECK if it has leading white space (probably trailing too?). Apparently prepared statements have those spaces trimmed out and so the current comparison between the input and returned statements fails.
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c0d58678bf84f546431064adb80df3fa7acf8948

commit c0d58678bf84f546431064adb80df3fa7acf8948
Author: Dan Harrington <harringtond@chromium.org>
Date: Fri Nov 16 15:28:27 2018

Fix DCHECK in second call to GetCachedStatement

When an SQL statement has leading or trailing whitespace, the stored
statement doesn't match when performing the DCHECK. Fixed by trimming whitespace
before comparing the statements.

Bug: 894884
Change-Id: If49b06103ed4fa3d49e623f62ce43247b2a27cf3
Reviewed-on: https://chromium-review.googlesource.com/c/1338177
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Commit-Queue: Dan H <harringtond@google.com>
Cr-Commit-Position: refs/heads/master@{#608787}
[modify] https://crrev.com/c0d58678bf84f546431064adb80df3fa7acf8948/sql/database.cc
[modify] https://crrev.com/c0d58678bf84f546431064adb80df3fa7acf8948/sql/database_unittest.cc

Comment 2 by jsb...@chromium.org, Nov 16

Status: Fixed (was: Untriaged)

Comment 3 by pwnall@chromium.org, Nov 19

Owner: pwnall@chromium.org
Status: Started (was: Fixed)
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/613b4307ff16548c7f23b8c225bcb5724603b9dc

commit 613b4307ff16548c7f23b8c225bcb5724603b9dc
Author: Victor Costan <pwnall@chromium.org>
Date: Tue Nov 20 05:32:43 2018

sql: Restore strict DCHECK of cached statements SQL.

https://crrev.com/c/1338177 relaxed the DCHECKs for
sql::Database::GetCachedStatment by allowing the SQL statement argument
to have leading and trailing whitespace when compared with the SQL
statement reported by SQLite.

This CL reverts the relaxed restriction. Instead, GetCachedStatement
will also DCHECK that the SQL statement argument matches the SQL
statement reported by SQLite when preparing the statement. This will
help developers discover non-canonical statements on first use.

Bug: 894884
Change-Id: I166f8cda8d5a3980d9858a26507f778bc87ee6cc
Reviewed-on: https://chromium-review.googlesource.com/c/1341493
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609605}
[modify] https://crrev.com/613b4307ff16548c7f23b8c225bcb5724603b9dc/sql/database.cc
[modify] https://crrev.com/613b4307ff16548c7f23b8c225bcb5724603b9dc/sql/database.h
[modify] https://crrev.com/613b4307ff16548c7f23b8c225bcb5724603b9dc/sql/database_unittest.cc

Comment 5 by carl...@google.com, Nov 28 

From this latest CL I am assuming that the trailing/leading white space case is being considered an unsupported way of writing a SQL statement. Is this interpretation correct?
►

Sign in to add a comment