sql: Small documentation improvements for SandboxedVfs.

Change-Id: I472582eaea658d2ee8a4c157a0161611d2aca2e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2359229Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Ken Rockot <rockot@google.com>
Auto-Submit: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798727}

sql/sandboxed_vfs.h

@@ -17,8 +17,9 @@
 
 namespace sql {
 
-// SQLite VFS implementation for use within a typical Chromium sandbox
-// environment. Instances are thread-friendly.
+// SQLite VFS file implementation that works in a sandboxed process.
+//
+// Instances are thread-friendly.
 class COMPONENT_EXPORT(SQL) SandboxedVfs {
  public:
   // Describes access rights for a path, used by Delegate::GetPathAccess below.
@@ -27,26 +28,31 @@ class COMPONENT_EXPORT(SQL) SandboxedVfs {
     bool can_write = false;
   };
 
-  // Interface which must be implemented by the environment using a
-  // SandboxedVfs. This abstracts a handful of operations that don't typically
-  // work in a sandbox environment given a typical naive implementation.
+  // Environment-specific SandboxedVfs implementation details.
   //
-  // Instances must be thread-friendly, as methods may be called from any
-  // thread.
+  // This abstracts a handful of operations that don't typically work in a
+  // sandbox environment given a typical naive implementation. Instances must be
+  // thread-safe.
   class Delegate {
    public:
     virtual ~Delegate() = default;
 
-    // Opens a file at the given `file_path`. `sqlite_requested_flags` is a
-    // bitwise combination SQLite flags used when opening files. Returns the
-    // opened File on success, or an invalid File on failure.
+    // Opens a file.
+    //
+    // `file_path` is the parsed version of a path passed by SQLite to Open().
+    // `sqlite_requested_flags` is a bitwise combination SQLite flags used when
+    // opening files. Returns the opened File on success, or an invalid File on
+    // failure.
     virtual base::File OpenFile(const base::FilePath& file_path,
                                 int sqlite_requested_flags) = 0;
 
-    // Deletes a file at the given `file_path`. If `sync_dir` is true, the
-    // implementation should synchronize the containing directory contents if
-    // possible. Returns an SQLite error code indicating the status of the
-    // operation.
+    // Deletes a file.
+    //
+    // `file_path` is the parsed version of a path passed by SQLite to Delete().
+    // If `sync_dir` is true, the implementation should attempt to flush to disk
+    // the changes to the file's directory, to ensure that the deletion is
+    // reflected after a power failure. Returns an SQLite error code indicating
+    // the status of the operation.
     virtual int DeleteFile(const base::FilePath& file_path, bool sync_dir) = 0;
 
     // Queries path access information for `file_path`. Returns null if the
@@ -54,14 +60,19 @@ class COMPONENT_EXPORT(SQL) SandboxedVfs {
     virtual base::Optional<PathAccessInfo> GetPathAccess(
         const base::FilePath& file_path) = 0;
 
-    // Resizes the file at `file_path` (opened in `file`) to `size` bytes,
-    // returning true if successful and false otherwise. Implementations may
-    // want to operate on the filesystem via `file_path`, or modify `file`
-    // directly if possible.
+    // Resizes a file.
+    //
+    // `file` is the result of a previous call to Delegate::OpenFile() with
+    // `file_path`. `size` is the new desired size in bytes, and may be smaller
+    // or larger than the current file size. Returns true if successful and
+    // false otherwise.
+    //
+    // Implementations can modify `file` directly, or operate on the filesystem
+    // via `file_path`.
     //
-    // Note that if this is called by the VFS, it's because a direct call to
-    // SetLength on `file` has already failed. The implementation should not
-    // attempt to repeat this.
+    // This is only called after the direct approach of base::File::SetLength()
+    // fails. So, the implementation should not bother trying to call
+    // SetLength() on `file`. This currently only happens on macOS < 10.15.
     virtual bool SetFileLength(const base::FilePath& file_path,
                                base::File& file,
                                size_t size) = 0;

sql/sandboxed_vfs_file.cc

@@ -215,8 +215,8 @@ int SandboxedVfsFile::Truncate(sqlite3_int64 size) {
   if (file_.SetLength(size))
     return SQLITE_OK;
 
-  // On Mac, the default sandbox blocks ftruncate(), so we have to use a sync
-  // mojo IPC to ask the browser process to call ftruncate() for us.
+  // On macOS < 10.15, the default sandbox blocks ftruncate(), so we have to use
+  // a sync mojo IPC to ask the browser process to call ftruncate() for us.
   //
   // TODO(crbug.com/1084565): Figure out if we can allow ftruncate() in renderer
   // and utility processes. It would be useful for low-level storage APIs, like
@@ -466,7 +466,13 @@ int SandboxedVfsFile::ShmMap(int page_index,
   DCHECK_GE(page_size, 0);
   DCHECK(result);
 
-  // NOTE: This would be needed by SQLite's WAL mode.
+  // https://www.sqlite.org/wal.html#use_of_wal_without_shared_memory states
+  // that SQLite only attempts to use shared memory "-shm" files for databases
+  // in WAL mode that may be accessed by multiple processes (are not EXCLUSIVE).
+  //
+  // Chrome will not only use WAL mode on EXCLUSIVE databases.
+  NOTREACHED() << "SQLite should not attempt to use shared memory";
+
   *result = nullptr;
   return SQLITE_IOERR;
 }
@@ -475,18 +481,37 @@ int SandboxedVfsFile::ShmLock(int offset, int size, int flags) {
   DCHECK_GE(offset, 0);
   DCHECK_GE(size, 0);
 
-  // NOTE: This would be needed by SQLite's WAL mode.
+  // https://www.sqlite.org/wal.html#use_of_wal_without_shared_memory states
+  // that SQLite only attempts to use shared memory "-shm" files for databases
+  // in WAL mode that may be accessed by multiple processes (are not EXCLUSIVE).
+  //
+  // Chrome will not only use WAL mode on EXCLUSIVE databases.
+  NOTREACHED() << "SQLite should not attempt to use shared memory";
+
   return SQLITE_IOERR;
 }
 
 void SandboxedVfsFile::ShmBarrier() {
+  // https://www.sqlite.org/wal.html#use_of_wal_without_shared_memory states
+  // that SQLite only attempts to use shared memory "-shm" files for databases
+  // in WAL mode that may be accessed by multiple processes (are not EXCLUSIVE).
+  //
+  // Chrome will not only use WAL mode on EXCLUSIVE databases.
+  NOTREACHED() << "SQLite should not attempt to use shared memory";
+
   // All writes to shared memory that have already been issued before this
   // function is called must complete before the function returns.
   std::atomic_thread_fence(std::memory_order_acq_rel);
 }
 
 int SandboxedVfsFile::ShmUnmap(int also_delete_file) {
-  // NOTE: This would be needed by SQLite's WAL mode.
+  // https://www.sqlite.org/wal.html#use_of_wal_without_shared_memory states
+  // that SQLite only attempts to use shared memory "-shm" files for databases
+  // in WAL mode that may be accessed by multiple processes (are not EXCLUSIVE).
+  //
+  // Chrome will not only use WAL mode on EXCLUSIVE databases.
+  NOTREACHED() << "SQLite should not attempt to use shared memory";
+
   return SQLITE_IOERR;
 }
 

sql/sandboxed_vfs_file.h

@@ -13,7 +13,7 @@ namespace sql {
 
 class SandboxedVfs;
 
-// SQLite VFS file implementation that works in a typical Chromium sandbox.
+// SQLite VFS file implementation that works in a sandboxed process.
 //
 // An instance is created when SQLite calls into SandboxedVfs::Open(). The
 // instance is deleted by a call to SandboxedVfsFile::Close().