[sql] Remove mmap before Raze().

[sql] Remove mmap before Raze().

On Windows, file truncation silently fails when the file is memory
mapped.  Modify Raze() to disable memory mapping before truncating.

BUG=none
R=pwnall@chromium.org

Review-Url: https://codereview.chromium.org/2830263002
Cr-Commit-Position: refs/heads/master@{#466555}
Committed: https://chromium.googlesource.com/chromium/src/+/92a6fb2bc85e4563090de085d19c39be8bc3b795

[Scott Hess - ex-Googler, 2017-04-21 05:39:01]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 05:39:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 06:52:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 06:52:24]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-04-21 11:00:14]

https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc#newcode1118
sql/connection.cc:1118: // prevent the problem.
Is there any reference for the Windows issue? I saw it mentioned in
https://sqlite.org/mmap.html but I couldn't find a different source. It'd be
nice to have something that allows us to track whether this is still an issue or
not on newer Windows versions.

Would it make sense to try to save the current mmap_size value and reload it
after the truncate?

Also, would it make sense to only have this workaround applied to Windows? #if
defined(OS_WINDOWS)?

[Scott Hess - ex-Googler, 2017-04-21 14:16:05]

ifdef OS_WIN

[Scott Hess - ex-Googler, 2017-04-21 14:16:17]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 14:16:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-04-21 14:18:41]

speeeling

[Scott Hess - ex-Googler, 2017-04-21 14:18:43]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[Scott Hess - ex-Googler, 2017-04-21 14:18:47]

https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc#newcode1118
sql/connection.cc:1118: // prevent the problem.
On 2017/04/21 11:00:14, pwnall wrote:
> Is there any reference for the Windows issue? I saw it mentioned in
> https://sqlite.org/mmap.html but I couldn't find a different source. It'd be
> nice to have something that allows us to track whether this is still an issue
or
> not on newer Windows versions.

I haven't seen anything better than what SQLite says.  But experimentally it
does seem to happen.

> Would it make sense to try to save the current mmap_size value and reload it
> after the truncate?

The primary use for Raze() is as part of "I give up" error handling.  In that
case, the next step will be to poison the database handle.  So it's hard to find
a plausible advantage to additional code.

[It may make sense to inline RazeAndClose(), but historically this developed as
"Here's a tool to help clear the database" and later "Here's a better way to
handle errors in that case".]

> Also, would it make sense to only have this workaround applied to Windows? #if
> defined(OS_WINDOWS)?

I can't think of any reason against an #if, since other platforms don't have
this truncate issue.  I think that having allowed the truncate to proceed,
security demands that they do the right thing with previously-mapped blocks past
the end of the truncated file, and Linux and OSX are both generally trustworthy
in this area.  Presumably failing the truncate is how Windows handles the
security issue.

Umm ... robost_ftruncate() has a not about Android only respecting 32-bit
offsets, so truncates beyond 2GB are dropped.  I don't think that applies
specifically to this (it's not mmap-specific), so ignoring.

[commit-bot: I haz the power, 2017-04-21 14:18:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 15:11:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 15:11:49]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-04-21 19:00:18]

LGTM w/ the comment below.

Also, thank you very much for doing this!

https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc#newcode1118
sql/connection.cc:1118: // prevent the problem.
On 2017/04/21 14:18:47, Scott Hess wrote:
> On 2017/04/21 11:00:14, pwnall wrote:
> > Is there any reference for the Windows issue? I saw it mentioned in
> > https://sqlite.org/mmap.html but I couldn't find a different source. It'd be
> > nice to have something that allows us to track whether this is still an
issue
> or
> > not on newer Windows versions.
> 
> I haven't seen anything better than what SQLite says.  But experimentally it
> does seem to happen.
> 
> > Would it make sense to try to save the current mmap_size value and reload it
> > after the truncate?
> 
> The primary use for Raze() is as part of "I give up" error handling.  In that
> case, the next step will be to poison the database handle.  So it's hard to
find
> a plausible advantage to additional code.
> 
> [It may make sense to inline RazeAndClose(), but historically this developed
as
> "Here's a tool to help clear the database" and later "Here's a better way to
> handle errors in that case".]
> 
> > Also, would it make sense to only have this workaround applied to Windows?
#if
> > defined(OS_WINDOWS)?
> 
> I can't think of any reason against an #if, since other platforms don't have
> this truncate issue.  I think that having allowed the truncate to proceed,
> security demands that they do the right thing with previously-mapped blocks
past
> the end of the truncated file, and Linux and OSX are both generally
trustworthy
> in this area.  Presumably failing the truncate is how Windows handles the
> security issue.
> 
> Umm ... robost_ftruncate() has a not about Android only respecting 32-bit
> offsets, so truncates beyond 2GB are dropped.  I don't think that applies
> specifically to this (it's not mmap-specific), so ignoring.

Oh, I was under the impression that the same connection might be reused after
the database is razed, dooming every platform to slower speeds because of
Windows' incompetence. If that's not the case, please discard my comment about
restoring mmap_size.

I would prefer that the workaround is Windows-only. No need to increase code
size and reason about more things on other platforms. If the test starts failing
(thank you for adding it!), we can revise this decision.

[Scott Hess - ex-Googler, 2017-04-23 03:34:17]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2017-04-23 03:34:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-04-23 03:36:09]

https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc#newcode1118
sql/connection.cc:1118: // prevent the problem.
On 2017/04/21 19:00:18, pwnall wrote:
> Oh, I was under the impression that the same connection might be reused after
> the database is razed, dooming every platform to slower speeds because of
> Windows' incompetence. If that's not the case, please discard my comment about
> restoring mmap_size.

I'm not aware of Raze() cases where the connection continues to be used, but it
should work, so it's possible I'm just not aware of them.  But the mmap settings
are not persisted, so any side effects of turning it off would only apply until
next restart.

> I would prefer that the workaround is Windows-only. No need to increase code
> size and reason about more things on other platforms. If the test starts
failing
> (thank you for adding it!), we can revise this decision.

OK, I think the workaround should be Windows-only, but I'll leave the test
cross-platform.

[commit-bot: I haz the power, 2017-04-23 04:33:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492918457056690,
"parent_rev": "8aa7a4e1cf5bb91918e8692a1909ededca93523b", "commit_rev":
"92a6fb2bc85e4563090de085d19c39be8bc3b795"}

[commit-bot: I haz the power, 2017-04-23 04:34:54]

Description was changed from

==========
[sql] Remove mmap before Raze().

On Windows, file truncation silently fails when the file is memory
mapped.  Modify Raze() to disable memory mapping before truncating.

BUG=none
R=pwnall@chromium.org
==========

to

==========
[sql] Remove mmap before Raze().

On Windows, file truncation silently fails when the file is memory
mapped.  Modify Raze() to disable memory mapping before truncating.

BUG=none
R=pwnall@chromium.org

Review-Url: https://codereview.chromium.org/2830263002
Cr-Commit-Position: refs/heads/master@{#466555}
Committed:
https://chromium.googlesource.com/chromium/src/+/92a6fb2bc85e4563090de085d19c...
==========

[commit-bot: I haz the power, 2017-04-23 04:34:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/92a6fb2bc85e4563090de085d19c...

[pwnall, 2017-04-23 09:12:42]

On 2017/04/23 03:36:09, Scott Hess wrote:
> OK, I think the workaround should be Windows-only, but I'll leave the test
> cross-platform.

Excellent! This is what I meant -- the test should definitely be cross-platform,
so we learn if the SQLite that we use diverges from our expectations in the
future.

Thank you very much for compensating for my lack of clarity!