[sqlite_vfs] Move sandboxed SQLite VFS to its own component [chromium/src : main]
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 1 comment![]( "Open in Gerrit")
grt@
Could you please review this CL?
Thank you.
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 2 comments![]( "Open in Gerrit")
sqlite_vfs::PendingBackend pending_backend);This change (the bulk-move of `PendingBackend` like this) locks `PersistentCache` into using this particular SQLite backend. This goes against my earlier remark that "persistent_cache::PendingBackend and its producers/consumers are designed to accommodate different types of backends. We would like to keep this in PC's object model."
Could you look into ways to change this split so that today's use of `PersistentCache` and `PendingBackend` aren't locked into SQLite like this?
Related details
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 2 comments![]( "Open in Gerrit")
sqlite_vfs::PendingBackend pending_backend);This change (the bulk-move of `PendingBackend` like this) locks `PersistentCache` into using this particular SQLite backend. This goes against my earlier remark that "persistent_cache::PendingBackend and its producers/consumers are designed to accommodate different types of backends. We would like to keep this in PC's object model."
Could you look into ways to change this split so that today's use of `PersistentCache` and `PendingBackend` aren't locked into SQLite like this?
To address the concern about backend flexibility, I have moved the SQLite-specific data into a new sqlite_vfs::PendingFileSet struct. persistent_cache::PendingBackend now contains this struct as a member. I believe this structure better accommodates future backend types. I would appreciate your further review.
Thank you.
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 17 comments![]( "Open in Gerrit")
hi. this is getting closer. thanks for working on it.
"pending_backend.h",since this file includes `components/sqlite_vfs/pending_file_set.h`, the `persistent_cache` target needs to list a target that provides that .h in its `public_deps`.
since we don't want `persistent_cache` consumers to use any other facilities of `sqlite_vfs`, is it possible to split the latter into two targets: one that only has pending_file_set.h in its `public` list and is in `public_deps` here, and another that has the full implementation and is in `deps` here? this would help prevent consumers of `persistent_cache` from trying to use other bits of `sqlite_vfs` directly.
sqlite_vfs::PendingFileSet pending_file_set;can we not keep using `PendingBackend` here?
"//mojo/public/mojom/base",no longer needed
deps = [ "//mojo/public/cpp/base:shared_typemap_traits" ]no longer needed
explicit PendingBackend(sqlite_vfs::PendingFileSet pending_file_set);can we do something to restrict access to this ctor so that it can only be used by the sqlite `BackendStorageDelegate` impl and the mojom deserializer? maybe something with PassKey or private/friend? maybe `pending_file_set` can also be private?
#include "components/sqlite_vfs/pending_file_set.h"i think we should strive to make this the only sqlte_vfs header/type that is needed by consumers of persistent_cache. it would be really nice, actually, if `PendingBackend` was completely opaque to consumers of persistent_cache. persistent_cache consumers should never need to reach into a `PB` instance -- they should just get them and pass them across mojo connections to be consumed elsewhere. in particular, a persistent_cache consumer should never use sqlite_vfs APIs to get a `PendingFileSet` from somewhere else and then put it into a `PendingBackend`.
not something i'm suggesting that you address here, just thinking out loud to share some design principles.
: pending_file_set(std::move(pending_file_set)) {}#include <utility>
sqlite_vfs::LockState Abandon();can we avoid leaking SQLite out through the API here? as it is, this forces all backends to use this SQLite-specific type.
(same comment applies in Backend interface)
auto pending_file_set = sqlite_vfs::ShareConnection(consider using `ASSIGN_OR_RETURN` from base/types/expected_macros.h. for example:
```
ASSIGN_OR_RETURN(auto pending_file_set,
sqlite_vfs::ShareConnection(...));
return PendingBackend(std::move(pending_file_set));
```
return pending_file_set ? PendingBackend(std::move(*pending_file_set))nit: when moving a value out of an optional, use `*std::move(pending_file_set)` as per go/totw/181#moving-from-the-value-of-an-abslstatusor
: std::optional<PendingBackend>();nit: use `std::nullopt` for an empty optional.
struct COMPONENT_EXPORT(SQLITE_VFS) SqliteData {do we need this nested type here? this was present in persistent_cache so that we can support different backend types there. sqlite_vfs is the implementation of one specific type of backend, so i don't think we need that flexibility here.
std::optional<SqliteVfsFileSet> BindToFileSet(PendingFileSet pending_file_set);now that `PendingFileSet` and `SqliteVfsFileSet` are in the same component, should we make this a `static` factory function in `SqliteVfsFileSet` itself? for example:
```
class SqliteVfsFileSet {
public:
static std::optional<SqliteVfsFileSet> Bind(PendingFileSet pending_file_set);
```
std::optional<PendingFileSet> ShareConnection(const base::FilePath& directory,this is a dangerous API, so i think we need some very scary text in the doc comment for it. previously, it was an implementation detail of `BackendStorageDelegate`, which isn't publicly accessible, so it was not possible for it to be misused.
the issue is that we can only satisfy a request for r/o access to a `file_set` that holds r/w handles on POSIX if we know the paths to the files. if a caller accidentally provides a different `directory` or `base_name` here compared to when `file_set` was created, then the returned `PendingFileSet` could actually point to different file(s). the only use of this in persistent_cache (PersistentCacheCollection::ShareReadOnlyConnection) is safe because it doesn't give the caller an opportunity to confuse matters.
at the very least, there should be some text in the comment saying that the caller **MUST** ensure that `directory` and `base_name` are identical to those used when `MakePendingFileSet` was called to obtain `file_set` in the first place.
#include "base/files/file_path.h"forward-declare `FilePath` and move this to .cc
Related details
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 17 comments![]( "Open in Gerrit")
Thank you
since this file includes `components/sqlite_vfs/pending_file_set.h`, the `persistent_cache` target needs to list a target that provides that .h in its `public_deps`.
since we don't want `persistent_cache` consumers to use any other facilities of `sqlite_vfs`, is it possible to split the latter into two targets: one that only has pending_file_set.h in its `public` list and is in `public_deps` here, and another that has the full implementation and is in `deps` here? this would help prevent consumers of `persistent_cache` from trying to use other bits of `sqlite_vfs` directly.
Done
sqlite_vfs::PendingFileSet pending_file_set;can we not keep using `PendingBackend` here?
Could you please clarify what you mean by "not keep using PendingBackend"? If you meant we should avoid directly using sqlite_vfs::PendingFileSet here, I have updated the code to no longer use it.
"//mojo/public/mojom/base",Tsuyoshi Horono longer needed
Done
deps = [ "//mojo/public/cpp/base:shared_typemap_traits" ]Tsuyoshi Horono longer needed
Done
explicit PendingBackend(sqlite_vfs::PendingFileSet pending_file_set);can we do something to restrict access to this ctor so that it can only be used by the sqlite `BackendStorageDelegate` impl and the mojom deserializer? maybe something with PassKey or private/friend? maybe `pending_file_set` can also be private?
Done
i think we should strive to make this the only sqlte_vfs header/type that is needed by consumers of persistent_cache. it would be really nice, actually, if `PendingBackend` was completely opaque to consumers of persistent_cache. persistent_cache consumers should never need to reach into a `PB` instance -- they should just get them and pass them across mojo connections to be consumed elsewhere. in particular, a persistent_cache consumer should never use sqlite_vfs APIs to get a `PendingFileSet` from somewhere else and then put it into a `PendingBackend`.
not something i'm suggesting that you address here, just thinking out loud to share some design principles.
Acknowledged
: pending_file_set(std::move(pending_file_set)) {}Tsuyoshi Horo#include <utility>
Done
can we avoid leaking SQLite out through the API here? as it is, this forces all backends to use this SQLite-specific type.
(same comment applies in Backend interface)
Modified this method and Backend::Abandon() and SqliteVfsFileSet::Abandon() return a bool.
consider using `ASSIGN_OR_RETURN` from base/types/expected_macros.h. for example:
```
ASSIGN_OR_RETURN(auto pending_file_set,
sqlite_vfs::ShareConnection(...));
return PendingBackend(std::move(pending_file_set));
```
Done
return pending_file_set ? PendingBackend(std::move(*pending_file_set))nit: when moving a value out of an optional, use `*std::move(pending_file_set)` as per go/totw/181#moving-from-the-value-of-an-abslstatusor
Done
nit: use `std::nullopt` for an empty optional.
Done
do we need this nested type here? this was present in persistent_cache so that we can support different backend types there. sqlite_vfs is the implementation of one specific type of backend, so i don't think we need that flexibility here.
Sounds good. Flattened the PendingFileSet struct.
std::optional<SqliteVfsFileSet> BindToFileSet(PendingFileSet pending_file_set);now that `PendingFileSet` and `SqliteVfsFileSet` are in the same component, should we make this a `static` factory function in `SqliteVfsFileSet` itself? for example:
```
class SqliteVfsFileSet {
public:
static std::optional<SqliteVfsFileSet> Bind(PendingFileSet pending_file_set);
```
Done
std::optional<PendingFileSet> ShareConnection(const base::FilePath& directory,this is a dangerous API, so i think we need some very scary text in the doc comment for it. previously, it was an implementation detail of `BackendStorageDelegate`, which isn't publicly accessible, so it was not possible for it to be misused.
the issue is that we can only satisfy a request for r/o access to a `file_set` that holds r/w handles on POSIX if we know the paths to the files. if a caller accidentally provides a different `directory` or `base_name` here compared to when `file_set` was created, then the returned `PendingFileSet` could actually point to different file(s). the only use of this in persistent_cache (PersistentCacheCollection::ShareReadOnlyConnection) is safe because it doesn't give the caller an opportunity to confuse matters.
at the very least, there should be some text in the comment saying that the caller **MUST** ensure that `directory` and `base_name` are identical to those used when `MakePendingFileSet` was called to obtain `file_set` in the first place.
Added comments.
forward-declare `FilePath` and move this to .cc
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 9 comments![]( "Open in Gerrit")
sqlite_vfs::PendingFileSet pending_file_set;Tsuyoshi Horocan we not keep using `PendingBackend` here?
Could you please clarify what you mean by "not keep using PendingBackend"? If you meant we should avoid directly using sqlite_vfs::PendingFileSet here, I have updated the code to no longer use it.
yup, that was what i meant. apologies for the confusing request. :-)
PendingBackend pending_backend;
pending_backend.read_write = true;this was here so that the expectation on line 91 can verify that the exact instance returned by the delegate is the one that ends up in `result` on line 88. is it possible to keep this as it was? as it is now in patch set 30, `backend_storage().MakePendingBackend(...)` could ignore the result from the delegate and return some other empty `PendingBackend`.
sqlite_vfs::LockState Abandon();Tsuyoshi Horocan we avoid leaking SQLite out through the API here? as it is, this forces all backends to use this SQLite-specific type.
(same comment applies in Backend interface)
Modified this method and Backend::Abandon() and SqliteVfsFileSet::Abandon() return a bool.
i'm checking with @oliv...@chromium.org about this. while the bool works for what we have here today, i don't remember if we were planning to handle the two cases differently someday.
if this change is okay, i would suggest making this API change in its own CL so that this and the sqlite_vfs extraction are independent.
"//base",this should be in a `public_deps` block since the public headers include files from //base.
// Marks the file as no longer suitable for use. Returns true if some
// connections had a view of the files while abandoning. Should only be usedthis isn't quite correct. true means that another connection to the file set held either a shared reader lock or the exclusive pending/reserved writer lock.
i think it's important that we make this comment a bit more verbose and precise since this is now becoming a public API (it was previously an implementation detail of Persistent Cache). i propose:
```
// Permanently marks this file set's database as no longer suitable for use by
// any connection. Returns true if any connection to the database holds either
// a shared reader lock; or the reserved, pending, or exclusive lock. All
// subsequent attempts to lock the database by any connection will fail with
// SQLITE_IOERR_LOCK. Clients accessing a database by such a file set should
// handle this error by closing their connection. When `Abandon()` returns
// false, it is safe to re-establish new connections to the same files.
// Conversely, the backing files should be deleted if a file set is abandoned
// while any other connection holds a lock since it is not possible to know
// when all outstanding connections have been closed.
```
does that make sense to you?
static std::optional<SqliteVfsFileSet> Bind(PendingFileSet pending_file_set);#include <optional>
base::File DuplicateFile(const base::File& source_file,#include "base/files/file.h"
CHECK(source_file.IsValid());#include "base/check.h"
Related details
- Olivier Li
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
sqlite_vfs::LockState Abandon();Tsuyoshi Horocan we avoid leaking SQLite out through the API here? as it is, this forces all backends to use this SQLite-specific type.
(same comment applies in Backend interface)
Greg ThompsonModified this method and Backend::Abandon() and SqliteVfsFileSet::Abandon() return a bool.
i'm checking with @oliv...@chromium.org about this. while the bool works for what we have here today, i don't remember if we were planning to handle the two cases differently someday.
if this change is okay, i would suggest making this API change in its own CL so that this and the sqlite_vfs extraction are independent.
Olivier points out that the tristate enables an optimization whereby a copy of the files can be used after `Abandon` if the lock was held only by readers.
one way to deal with this is to simply have LockState in both namespaces. mapping from one to the other could be done via `switch` or direct casting w/ some `static_assert`s to ensure at compile time that the values match.
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo voted and added 10 comments![]( "Open in Gerrit")
Votes added by Tsuyoshi Horo
| Commit-Queue | +0 |
10 comments
PendingBackend pending_backend;
pending_backend.read_write = true;this was here so that the expectation on line 91 can verify that the exact instance returned by the delegate is the one that ends up in `result` on line 88. is it possible to keep this as it was? as it is now in patch set 30, `backend_storage().MakePendingBackend(...)` could ignore the result from the delegate and return some other empty `PendingBackend`.
Done
sqlite_vfs::LockState Abandon();Tsuyoshi Horocan we avoid leaking SQLite out through the API here? as it is, this forces all backends to use this SQLite-specific type.
(same comment applies in Backend interface)
Greg ThompsonModified this method and Backend::Abandon() and SqliteVfsFileSet::Abandon() return a bool.
Greg Thompsoni'm checking with @oliv...@chromium.org about this. while the bool works for what we have here today, i don't remember if we were planning to handle the two cases differently someday.
if this change is okay, i would suggest making this API change in its own CL so that this and the sqlite_vfs extraction are independent.
Olivier points out that the tristate enables an optimization whereby a copy of the files can be used after `Abandon` if the lock was held only by readers.
one way to deal with this is to simply have LockState in both namespaces. mapping from one to the other could be done via `switch` or direct casting w/ some `static_assert`s to ensure at compile time that the values match.
Thanks!
Done via `switch`.
this should be in a `public_deps` block since the public headers include files from //base.
Done
This change
Acknowledged
// Marks the file as no longer suitable for use. Returns true if some
// connections had a view of the files while abandoning. Should only be usedthis isn't quite correct. true means that another connection to the file set held either a shared reader lock or the exclusive pending/reserved writer lock.
i think it's important that we make this comment a bit more verbose and precise since this is now becoming a public API (it was previously an implementation detail of Persistent Cache). i propose:
```
// Permanently marks this file set's database as no longer suitable for use by
// any connection. Returns true if any connection to the database holds either
// a shared reader lock; or the reserved, pending, or exclusive lock. All
// subsequent attempts to lock the database by any connection will fail with
// SQLITE_IOERR_LOCK. Clients accessing a database by such a file set should
// handle this error by closing their connection. When `Abandon()` returns
// false, it is safe to re-establish new connections to the same files.
// Conversely, the backing files should be deleted if a file set is abandoned
// while any other connection holds a lock since it is not possible to know
// when all outstanding connections have been closed.
```
does that make sense to you?
Done.
static std::optional<SqliteVfsFileSet> Bind(PendingFileSet pending_file_set);Tsuyoshi Horo#include <optional>
Done
base::File DuplicateFile(const base::File& source_file,Tsuyoshi Horo#include "base/files/file.h"
Done
CHECK(source_file.IsValid());Tsuyoshi Horo#include "base/check.h"
Done
Related details
- Greg Thompson
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Greg Thompson
| Code-Review | +1 |
3 comments
lgtm with two nits. thanks for the extra changes.
auto pending_file_set = sqlite_vfs::MakePendingFileSet(
directory, base_name, single_connection, journal_mode_wal);
if (!pending_file_set) {
return std::nullopt;
}
return PendingBackend(std::move(*pending_file_set));nit:
```
ASSIGN_OR_RETURN(
auto pending_file_set,
sqlite_vfs::MakePendingFileSet(directory, base_name, single_connection,
journal_mode_wal));
return PendingBackend(std::move(pending_file_set));
```
or, if you prefer to not use the macro, please switch to `*std::move(pending_file_set)` on the last line.
auto instance = base::WrapUnique(new SqliteBackendImpl(std::move(*file_set)));nit: `*std::move(file_set)`
Related details
- Olivier Li
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 3 comments![]( "Open in Gerrit")
auto pending_file_set = sqlite_vfs::MakePendingFileSet(
directory, base_name, single_connection, journal_mode_wal);
if (!pending_file_set) {
return std::nullopt;
}
return PendingBackend(std::move(*pending_file_set));nit:
```
ASSIGN_OR_RETURN(
auto pending_file_set,
sqlite_vfs::MakePendingFileSet(directory, base_name, single_connection,
journal_mode_wal));
return PendingBackend(std::move(pending_file_set));
```
or, if you prefer to not use the macro, please switch to `*std::move(pending_file_set)` on the last line.
Done
auto instance = base::WrapUnique(new SqliteBackendImpl(std::move(*file_set)));Tsuyoshi Horonit: `*std::move(file_set)`
Done
Related details
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 1 comment![]( "Open in Gerrit")
Hi Olivier, could you please review this CL as an OWNER?
Thanks!
Related details
- Olivier Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li added 4 comments![]( "Open in Gerrit")
Looks good to me 😊
Please adress the comment about metric names before committing with a TODO or a fix.
Thanks!
# persistent_cache is not expected to depend on other components (exceptNit:
Might be a bit clearer to say:
`persistent_cache is expectly to depend only on sqlite_vfs ... `
// Note: This enum is currently a copy of sqlite_vfs::LockState.Could this be enforced with `LINT.ThenChange`?
base::UmaHistogramExactLinear("PersistentCache.Sqlite.DbFile.CreateResult",For now this is not going to change much but can we please make sure that this is renamed properly when PersistentCache is not the only code calling the function. Preferably the would be a prefix for PersitentCache and one for HttpCache.
Related details
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 4 comments![]( "Open in Gerrit")
# persistent_cache is not expected to depend on other components (exceptNit:
Might be a bit clearer to say:
`persistent_cache is expectly to depend only on sqlite_vfs ... `
Done
// Note: This enum is currently a copy of sqlite_vfs::LockState.Could this be enforced with `LINT.ThenChange`?
Done
base::UmaHistogramExactLinear("PersistentCache.Sqlite.DbFile.CreateResult",For now this is not going to change much but can we please make sure that this is renamed properly when PersistentCache is not the only code calling the function. Preferably the would be a prefix for PersitentCache and one for HttpCache.
Done. I've added TODO comments in several places.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 1 comment![]( "Open in Gerrit")
nyquist@
Could you please review this CL as an OWNER of `//components/`? This CL refactors the sandboxed SQLite Virtual File System (VFS) implementation in `//components/persistent_cache` to make it reusable by other components and adds a new component, `//components/sqlite_vfs`.
Thank you.
Related details
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 1 comment![]( "Open in Gerrit")
Please review files under `components/persistent_cache/mojom/` and `components/sqlite_vfs/mojom/`.
Thank you.
Related details
- Chromium IPC Reviews
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: hide...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): hide...@chromium.org
Reviewer source(s):
hide...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Hidehiko Abe
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |