sql: Rework bind parameter index checking.

Moving the checks inside Bind*() methods makes the DCHECK error
messages a bit easier to read.

Change-Id: Ia9a1fc81a2a74886517e660967b7c1a4313dbeed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3020323
Commit-Queue: Victor Costan <pwnall@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Auto-Submit: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#900808}

sql/statement.cc

@@ -139,76 +139,113 @@ bool Statement::Succeeded() const {
   return is_valid() && succeeded_;
 }
 
-bool Statement::BindNull(int col) {
+bool Statement::BindNull(int param_index) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
   DCHECK(!stepped_);
+  if (!is_valid())
+    return false;
 
-  return is_valid() && CheckOk(sqlite3_bind_null(ref_->stmt(), col + 1));
+  DCHECK_GE(param_index, 0);
+  DCHECK_LT(param_index, sqlite3_bind_parameter_count(ref_->stmt()))
+      << "Invalid parameter index";
+  return sqlite3_bind_null(ref_->stmt(), param_index + 1) == SQLITE_OK;
 }
 
-bool Statement::BindBool(int col, bool val) {
+bool Statement::BindBool(int param_index, bool val) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
 
-  return BindInt(col, val ? 1 : 0);
+  return BindInt64(param_index, val ? 1 : 0);
 }
 
-bool Statement::BindInt(int col, int val) {
+bool Statement::BindInt(int param_index, int val) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
   DCHECK(!stepped_);
+  if (!is_valid())
+    return false;
 
-  return is_valid() && CheckOk(sqlite3_bind_int(ref_->stmt(), col + 1, val));
+  DCHECK_GE(param_index, 0);
+  DCHECK_LT(param_index, sqlite3_bind_parameter_count(ref_->stmt()))
+      << "Invalid parameter index";
+  return sqlite3_bind_int(ref_->stmt(), param_index + 1, val) == SQLITE_OK;
 }
 
-bool Statement::BindInt64(int col, int64_t val) {
+bool Statement::BindInt64(int param_index, int64_t val) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
   DCHECK(!stepped_);
+  if (!is_valid())
+    return false;
 
-  return is_valid() && CheckOk(sqlite3_bind_int64(ref_->stmt(), col + 1, val));
+  DCHECK_GE(param_index, 0);
+  DCHECK_LT(param_index, sqlite3_bind_parameter_count(ref_->stmt()))
+      << "Invalid parameter index";
+  return sqlite3_bind_int64(ref_->stmt(), param_index + 1, val) == SQLITE_OK;
 }
 
-bool Statement::BindDouble(int col, double val) {
+bool Statement::BindDouble(int param_index, double val) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
   DCHECK(!stepped_);
+  if (!is_valid())
+    return false;
 
-  return is_valid() && CheckOk(sqlite3_bind_double(ref_->stmt(), col + 1, val));
+  DCHECK_GE(param_index, 0);
+  DCHECK_LT(param_index, sqlite3_bind_parameter_count(ref_->stmt()))
+      << "Invalid parameter index";
+  return sqlite3_bind_double(ref_->stmt(), param_index + 1, val) == SQLITE_OK;
 }
 
-bool Statement::BindTime(int col, base::Time val) {
+bool Statement::BindTime(int param_index, base::Time val) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
   DCHECK(!stepped_);
+  if (!is_valid())
+    return false;
 
+  DCHECK_GE(param_index, 0);
+  DCHECK_LT(param_index, sqlite3_bind_parameter_count(ref_->stmt()))
+      << "Invalid parameter index";
   int64_t int_value = val.ToDeltaSinceWindowsEpoch().InMicroseconds();
-  return is_valid() &&
-         CheckOk(sqlite3_bind_int64(ref_->stmt(), col + 1, int_value));
+  return sqlite3_bind_int64(ref_->stmt(), param_index + 1, int_value) ==
+         SQLITE_OK;
 }
 
-bool Statement::BindCString(int col, const char* val) {
+bool Statement::BindCString(int param_index, const char* val) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
   DCHECK(!stepped_);
+  DCHECK(val);
+  if (!is_valid())
+    return false;
 
-  return is_valid() && CheckOk(sqlite3_bind_text(ref_->stmt(), col + 1, val, -1,
-                                                 SQLITE_TRANSIENT));
+  DCHECK_GE(param_index, 0);
+  DCHECK_LT(param_index, sqlite3_bind_parameter_count(ref_->stmt()))
+      << "Invalid parameter index";
+  return sqlite3_bind_text(ref_->stmt(), param_index + 1, val, -1,
+                           SQLITE_TRANSIENT) == SQLITE_OK;
 }
 
-bool Statement::BindString(int col, base::StringPiece value) {
+bool Statement::BindString(int param_index, base::StringPiece value) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
   DCHECK(!stepped_);
+  if (!is_valid())
+    return false;
+
+  DCHECK_GE(param_index, 0);
+  DCHECK_LT(param_index, sqlite3_bind_parameter_count(ref_->stmt()))
+      << "Invalid parameter index";
 
   // base::StringPiece::data() may return null for empty pieces. In particular,
   // this may happen when the StringPiece is created from the default
@@ -219,24 +256,29 @@ bool Statement::BindString(int col, base::StringPiece value) {
   static constexpr char kEmptyPlaceholder[] = {0x00};
   const char* data = (value.size() > 0) ? value.data() : kEmptyPlaceholder;
 
-  return is_valid() &&
-         CheckOk(sqlite3_bind_text(ref_->stmt(), col + 1, data, value.size(),
-                                   SQLITE_TRANSIENT));
+  return sqlite3_bind_text(ref_->stmt(), param_index + 1, data, value.size(),
+                           SQLITE_TRANSIENT) == SQLITE_OK;
 }
 
-bool Statement::BindString16(int col, base::StringPiece16 value) {
+bool Statement::BindString16(int param_index, base::StringPiece16 value) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
 
-  return BindString(col, base::UTF16ToUTF8(value));
+  return BindString(param_index, base::UTF16ToUTF8(value));
 }
 
-bool Statement::BindBlob(int col, base::span<const uint8_t> value) {
+bool Statement::BindBlob(int param_index, base::span<const uint8_t> value) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #endif  // OS_ANDROID
   DCHECK(!stepped_);
+  if (!is_valid())
+    return false;
+
+  DCHECK_GE(param_index, 0);
+  DCHECK_LT(param_index, sqlite3_bind_parameter_count(ref_->stmt()))
+      << "Invalid parameter index";
 
   // span::data() may return null for empty spans. In particular, this may
   // happen when the span is created out of a std::vector, because
@@ -251,9 +293,8 @@ bool Statement::BindBlob(int col, base::span<const uint8_t> value) {
   static constexpr uint8_t kEmptyPlaceholder[] = {0x00};
   const uint8_t* data = (value.size() > 0) ? value.data() : kEmptyPlaceholder;
 
-  return is_valid() &&
-         CheckOk(sqlite3_bind_blob(ref_->stmt(), col + 1, data, value.size(),
-                                   SQLITE_TRANSIENT));
+  return sqlite3_bind_blob(ref_->stmt(), param_index + 1, data, value.size(),
+                           SQLITE_TRANSIENT) == SQLITE_OK;
 }
 
 int Statement::ColumnCount() const {
@@ -460,18 +501,6 @@ const char* Statement::GetSQLStatement() {
   return sqlite3_sql(ref_->stmt());
 }
 
-bool Statement::CheckOk(int err) const {
-#if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-#endif  // OS_ANDROID
-
-  // Binding to a non-existent variable is evidence of a serious error.
-  // TODO(gbillock,shess): make this invalidate the statement so it
-  // can't wreak havoc.
-  DCHECK_NE(err, SQLITE_RANGE) << "Bind value out of range";
-  return err == SQLITE_OK;
-}
-
 int Statement::CheckError(int err) {
 #if !defined(OS_ANDROID)  // TODO(crbug.com/866218): Remove this conditional
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

sql/statement.h

@@ -117,24 +117,25 @@ class COMPONENT_EXPORT(SQL) Statement {
 
   // Binding -------------------------------------------------------------------
 
-  // These all take a 0-based argument index and return true on success. You
+  // These all take a 0-based parameter index and return true on success. You
   // may not always care about the return value (they'll DCHECK if they fail).
   // The main thing you may want to check is when binding large blobs or
   // strings there may be out of memory.
-  bool BindNull(int col);
-  bool BindBool(int col, bool val);
-  bool BindInt(int col, int val);
-  bool BindInt(int col, int64_t val) = delete;  // Call BindInt64() instead.
-  bool BindInt64(int col, int64_t val);
-  bool BindDouble(int col, double val);
-  bool BindCString(int col, const char* val);
-  bool BindString(int col, base::StringPiece val);
-  bool BindString16(int col, base::StringPiece16 value);
-  bool BindBlob(int col, base::span<const uint8_t> value);
+  bool BindNull(int param_index);
+  bool BindBool(int param_index, bool val);
+  bool BindInt(int param_index, int val);
+  bool BindInt(int param_index,
+               int64_t val) = delete;  // Call BindInt64() instead.
+  bool BindInt64(int param_index, int64_t val);
+  bool BindDouble(int param_index, double val);
+  bool BindCString(int param_index, const char* val);
+  bool BindString(int param_index, base::StringPiece val);
+  bool BindString16(int param_index, base::StringPiece16 value);
+  bool BindBlob(int param_index, base::span<const uint8_t> value);
 
   // Overload that makes it easy to pass in std::string values.
-  bool BindBlob(int col, base::span<const char> value) {
-    return BindBlob(col, base::as_bytes(base::make_span(value)));
+  bool BindBlob(int param_index, base::span<const char> value) {
+    return BindBlob(param_index, base::as_bytes(base::make_span(value)));
   }
 
   // Conforms with base::Time serialization recommendations.
@@ -149,7 +150,7 @@ class COMPONENT_EXPORT(SQL) Statement {
   //
   // TODO(crbug.com/1195962): Migrate all time serialization to this method, and
   //                          then remove the migration details above.
-  bool BindTime(int col, base::Time time);
+  bool BindTime(int param_index, base::Time time);
 
   // Retrieving ----------------------------------------------------------------
 
@@ -207,10 +208,6 @@ class COMPONENT_EXPORT(SQL) Statement {
   // enhanced in the future to do the notification.
   int CheckError(int err);
 
-  // Contraction for checking an error code against SQLITE_OK. Does not set the
-  // succeeded flag.
-  bool CheckOk(int err) const;
-
   // Should be called by all mutating methods to check that the statement is
   // valid. Returns true if the statement is valid. DCHECKS and returns false
   // if it is not.