Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1006423008:
Add SetFileSize() IPC for WebSQL. (Closed)
|
Created:
5 years, 9 months ago by Scott Hess - ex-Googler Modified:
5 years, 8 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd SetFileSize() IPC for WebSQL.
The sandbox on OSX and Linux restricts ftruncate(), which SQLite uses.
Provide a browser hook for chromium_vfs.
BUG=457905
Committed: https://crrev.com/10ce3cc97b63b6e2a294f8279490e91ed22f06f4
Cr-Commit-Position: refs/heads/master@{#323925}
Patch Set 1 #
Total comments: 12
Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : Remove misleading comment. #
Total comments: 4
Patch Set 4 : Remove unnecessary IPC_MESSAGE_HANDLER_DELAY_REPLYs, remove unnecessary scoped_refptrs. #Messages
Total messages: 42 (13 generated)
Renderer side of the patch: https://codereview.chromium.org/1012353002/
shess@chromium.org changed reviewers: + michaeln@chromium.org
I think this is getting into the right region for proxying the xTruncate call for SQLite in WebSQL.
https://codereview.chromium.org/1006423008/diff/1/storage/browser/database/vf... File storage/browser/database/vfs_backend.cc (right): https://codereview.chromium.org/1006423008/diff/1/storage/browser/database/vf... storage/browser/database/vfs_backend.cc:160: // TODO(shess): Is there a base::File sitting around to do this? Otherwise it There is not, IPC::TakeFileHandleForProcess(file.Pass(),...) is used to xfer the handle to the renderer. https://codereview.chromium.org/1006423008/diff/1/storage/browser/database/vf... storage/browser/database/vfs_backend.cc:167: base::File file = base::File(file_path, flags); Depending on when/how xTruncate is used, seems like this SetLength function could fail due to sqlite having write locks, but ftruncate() would succeed if the open fd were passed directly to it. Is that possible and does sqlite try to use xTruncate at times when it would get tripped up by that?
https://codereview.chromium.org/1006423008/diff/1/storage/browser/database/vf... File storage/browser/database/vfs_backend.cc (right): https://codereview.chromium.org/1006423008/diff/1/storage/browser/database/vf... storage/browser/database/vfs_backend.cc:167: base::File file = base::File(file_path, flags); On 2015/03/19 00:36:33, michaeln wrote: > Depending on when/how xTruncate is used, seems like this SetLength function > could fail due to sqlite having write locks, but ftruncate() would succeed if > the open fd were passed directly to it. Is that possible and does sqlite try to > use xTruncate at times when it would get tripped up by that? On POSIX, locks are advisory only, so the file descriptors can be in both places just fine. This kind of problem could occur on Windows, though, I think. I think there are a couple options to get this right in the long term, worst case would be to keep a little platform-specific code to handle the locking-vs-sandbox distinctions.
lgtm https://codereview.chromium.org/1006423008/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/1006423008/diff/1/content/browser/renderer_ho... content/browser/renderer_host/database_message_filter.cc:203: if (!db_tracker_->HasSavedIncognitoFileHandle(vfs_file_name) && Not sure you'd want to change anything about the CL because of this, but there are existing file handles in the incogonito case. https://codereview.chromium.org/1006423008/diff/1/content/child/blink_platfor... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/1006423008/diff/1/content/child/blink_platfor... content/child/blink_platform_impl.h:64: const blink::WebString& vfs_file_name, long long size); i don't understand why this won't compile without having this method defined for blink::Platform? https://codereview.chromium.org/1006423008/diff/1/storage/browser/database/vf... File storage/browser/database/vfs_backend.cc (right): https://codereview.chromium.org/1006423008/diff/1/storage/browser/database/vf... storage/browser/database/vfs_backend.cc:160: // TODO(shess): Is there a base::File sitting around to do this? Otherwise it On 2015/03/19 00:36:33, michaeln wrote: > There is not, IPC::TakeFileHandleForProcess(file.Pass(),...) is used to xfer the > handle to the renderer. Except for incognito profiles.
https://codereview.chromium.org/1006423008/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/1006423008/diff/1/content/browser/renderer_ho... content/browser/renderer_host/database_message_filter.cc:203: if (!db_tracker_->HasSavedIncognitoFileHandle(vfs_file_name) && On 2015/04/01 21:04:48, michaeln wrote: > Not sure you'd want to change anything about the CL because of this, but there > are existing file handles in the incogonito case. I'm not even sure how this could be firing, because I don't think WAL would work right with chromium_vfs. https://codereview.chromium.org/1006423008/diff/1/content/child/blink_platfor... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/1006423008/diff/1/content/child/blink_platfor... content/child/blink_platform_impl.h:64: const blink::WebString& vfs_file_name, long long size); On 2015/04/01 21:04:48, michaeln wrote: > i don't understand why this won't compile without having this method defined for > blink::Platform? I was pretty sure it was? Unfortunately my blink checkout is failing somewhere else, so I can't tell you immediately where it was failing. https://codereview.chromium.org/1006423008/diff/1/storage/browser/database/vf... File storage/browser/database/vfs_backend.cc (right): https://codereview.chromium.org/1006423008/diff/1/storage/browser/database/vf... storage/browser/database/vfs_backend.cc:160: // TODO(shess): Is there a base::File sitting around to do this? Otherwise it On 2015/04/01 21:04:48, michaeln wrote: > On 2015/03/19 00:36:33, michaeln wrote: > > There is not, IPC::TakeFileHandleForProcess(file.Pass(),...) is used to xfer > the > > handle to the renderer. > > Except for incognito profiles. Seems like I should change the comment either way :-).
https://codereview.chromium.org/1006423008/diff/1/content/child/blink_platfor... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/1006423008/diff/1/content/child/blink_platfor... content/child/blink_platform_impl.h:64: const blink::WebString& vfs_file_name, long long size); On 2015/04/01 22:42:20, Scott Hess wrote: > On 2015/04/01 21:04:48, michaeln wrote: > > i don't understand why this won't compile without having this method defined > for > > blink::Platform? > > I was pretty sure it was? Unfortunately my blink checkout is failing somewhere > else, so I can't tell you immediately where it was failing. Sorry, the error doesn't come here, it's in a subclass: ../../content/renderer/renderer_blink_platform_impl.h:96:61: error: [chromium-style] Overriding method must be marked with 'override' or 'final'. My guess is that the warning system knows about the difference between blink-side and Chromiun-side classes, and since the blink interface doesn't define this method it complains.
https://codereview.chromium.org/1006423008/diff/1/content/renderer/renderer_b... File content/renderer/renderer_blink_platform_impl.h (right): https://codereview.chromium.org/1006423008/diff/1/content/renderer/renderer_b... content/renderer/renderer_blink_platform_impl.h:95: const blink::WebString& vfs_file_name, long long size); oh, is it saying to use 'override' here instead of 'virtual' since you're overriding the BlinkPlatformImpl method?
The CQ bit was checked by shess@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/1006423008/#ps20001 (title: "rebase")
OK, updated this and blink side ( https://codereview.chromium.org/1012353002/ ) and everything checks out, so ready to move forward. The only changes since your l-g-t-m were a couple whitespace changes and removing a comment which I think was mis-leading. But I'll not act on the stale approval for a bit in case you have a comment on the incognito discussion below. https://codereview.chromium.org/1006423008/diff/1/content/renderer/renderer_b... File content/renderer/renderer_blink_platform_impl.h (right): https://codereview.chromium.org/1006423008/diff/1/content/renderer/renderer_b... content/renderer/renderer_blink_platform_impl.h:95: const blink::WebString& vfs_file_name, long long size); On 2015/04/01 23:43:26, michaeln wrote: > oh, is it saying to use 'override' here instead of 'virtual' since you're > overriding the BlinkPlatformImpl method? I think that if it's a Platform method, it's fine, but if it's only in BlinkPlatformImpl, it's not fine. The Platform.h change landed, so np at this point. https://codereview.chromium.org/1006423008/diff/20001/content/browser/rendere... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/1006423008/diff/20001/content/browser/rendere... content/browser/renderer_host/database_message_filter.cc:300: success = VfsBackend::SetFileSize(db_file, size); If I understand things correctly, this is where I'll have to handle incognito profiles, right? AFAICT, with this change as-is, xTruncate() will change from failing in os_unix.c due to sandbox, to failing here due to not being implemented. So incognito should have the same failure before and after. Mostly I think I'd rather land this sooner to get the chromium_vfs changes baking, in case they have a fatal flaw. Also I suspect it'll take me a bit of code-reading and experimentation to figure out how incognito works in here :-). Right now I _think_ that incognito will only need a change here, to dig into the tracker and find the right handle to call SetLength() against. So it shouldn't be too nasty, and Chromium-side only (unless I need to land a layout test for incognito).
https://codereview.chromium.org/1006423008/diff/20001/content/browser/rendere... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/1006423008/diff/20001/content/browser/rendere... content/browser/renderer_host/database_message_filter.cc:300: success = VfsBackend::SetFileSize(db_file, size); On 2015/04/02 22:52:41, Scott Hess wrote: > If I understand things correctly, this is where I'll have to handle incognito > profiles, right? AFAICT, with this change as-is, xTruncate() will change from > failing in os_unix.c due to sandbox, to failing here due to not being > implemented. So incognito should have the same failure before and after. still lgtm, incognito behavior should be the same before and after > Mostly I think I'd rather land this sooner to get the chromium_vfs changes > baking, in case they have a fatal flaw. Also I suspect it'll take me a bit of > code-reading and experimentation to figure out how incognito works in here :-). > > Right now I _think_ that incognito will only need a change here, to dig into the > tracker and find the right handle to call SetLength() against. So it shouldn't > be too nasty, and Chromium-side only (unless I need to land a layout test for > incognito). File* incog_file = db_tracker_->GetIncognitoFile(vfs_file_name);
The CQ bit was checked by shess@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006423008/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
shess@chromium.org changed reviewers: + jam@chromium.org
jam for content/OWNERS . This is part of my bid to remove some brittle SQLite patches, since I've lost hope that we'll be able to remove all the WebSQL code on any specific timeframe.
https://codereview.chromium.org/1006423008/diff/40001/content/browser/rendere... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/1006423008/diff/40001/content/browser/rendere... content/browser/renderer_host/database_message_filter.cc:303: Send(reply_msg); this method, and a few others in this class, are using IPC_MESSAGE_HANDLER_DELAY_REPLY but don't need to. IPC_MESSAGE_HANDLER_DELAY_REPLY should only be used when the reply is written to asynchronously. https://codereview.chromium.org/1006423008/diff/40001/content/child/database_... File content/child/database_util.cc (right): https://codereview.chromium.org/1006423008/diff/40001/content/child/database_... content/child/database_util.cc:75: scoped_refptr<IPC::SyncMessageFilter> filter(sync_message_filter); why is this local ref taken? i.e. why isn't the reference held by whoever is calling this method sufficient?
The CQ bit was checked by shess@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/1006423008/#ps60001 (title: "Remove unnecessary IPC_MESSAGE_HANDLER_DELAY_REPLYs, remove unnecessary scoped_refptrs.")
Thanks for the input, John, PTAL? https://codereview.chromium.org/1006423008/diff/40001/content/browser/rendere... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/1006423008/diff/40001/content/browser/rendere... content/browser/renderer_host/database_message_filter.cc:303: Send(reply_msg); On 2015/04/03 15:14:23, jam wrote: > this method, and a few others in this class, are using > IPC_MESSAGE_HANDLER_DELAY_REPLY but don't need to. > IPC_MESSAGE_HANDLER_DELAY_REPLY should only be used when the reply is written to > asynchronously. Apologies, I was just copy/pasting from the parallel method. Changed OpenFile, GetFileAttributes, GetFileSize and SetFileSize to not be _DELAY_REPLY. https://codereview.chromium.org/1006423008/diff/40001/content/child/database_... File content/child/database_util.cc (right): https://codereview.chromium.org/1006423008/diff/40001/content/child/database_... content/child/database_util.cc:75: scoped_refptr<IPC::SyncMessageFilter> filter(sync_message_filter); On 2015/04/03 15:14:23, jam wrote: > why is this local ref taken? i.e. why isn't the reference held by whoever is > calling this method sufficient? I can't see why any of the refs in this file are necessary.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006423008/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by shess@chromium.org
On 2015/04/03 19:24:16, jam wrote: > lgtm Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006423008/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
shess@chromium.org changed reviewers: + jschuh@chromium.org
Guess I didn't read the OWNERS files sufficiently closely. Justin, could you explain where my sandbox escape is?
Two questions: 1) Is there a reason we can't limit the file size? 2) Should we be worried that any of these DB files can be arbitrarily resized if you know the name (or is there something blocking that)?
On 2015/04/03 21:52:10, jschuh wrote: > Two questions: > 1) Is there a reason we can't limit the file size? > 2) Should we be worried that any of these DB files can be arbitrarily resized > if you know the name (or is there something blocking that)? The things you can access here should be gated by GetFullFilePathForVfsFile(), so you should already be able to set arbitrarily large sizes by calling OpenFile() and writing data to the returned file handle (you could use GetFileSize and write zeros after the end, even). Since SQLite emulates pwrite for I/O, you could probably also open the file, seek to a distant point, and write something, and nobody would say boo. With previous code to shrink you'd have to delete and rewrite, which wouldn't work on Windows if there were open file handles, and would dissociate existing file handles on POSIX, so that's a new piece. Since the clients using the file data are all renderer side, I'm not sure how interesting that would be. I guess you could reach across renderers, but I suspect you could already do that using the right combination of faked -journal data. On the surface it might be that there could be a restriction tied into the quota system, but AFAICT the quota system lives above this code and isn't hardened in that way (I/O goes directly to the file handle, after all, with the quota being advisory to SQLite). SQLite's pager_truncate() uses sqlite3OsTruncate() to shrink and sqlite3OsWrite() at an offset to expand. AFAICT the most part other calls are only to shrink-if-needed. So I guess it could query the current size and only allow to shrink for this API?
I'd prefer it be limited to shrinking. However, for that to matter it sounds like we need to add some sort of sane bounds on creation. So, could you open a new bug for that with me CC'd, and I can lgtm this one for IPC security since it doesn't make anything worse.
The CQ bit was checked by shess@chromium.org
On 2015/04/06 16:26:30, jschuh wrote: > I'd prefer it be limited to shrinking. However, for that to matter it sounds > like we need to add some sort of sane bounds on creation. So, could you open a > new bug for that with me CC'd, and I can lgtm this one for IPC security since it > doesn't make anything worse. Bug created! http://crbug.com/474257
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006423008/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/10ce3cc97b63b6e2a294f8279490e91ed22f06f4 Cr-Commit-Position: refs/heads/master@{#323925} |
