[sqlite3] Limit max number of memory pages for fuzzing builds.

[sqlite3] Limit max number of memory pages for fuzzing builds.

R=shess@chromium.org
BUG=675446
Review-Url: https://codereview.chromium.org/2609473004
Cr-Commit-Position: refs/heads/master@{#467984}
Committed: https://chromium.googlesource.com/chromium/src/+/927053fa078035ee9bea31ef77184ccbe8001745

[Scott Hess - ex-Googler, 2017-04-27 20:22:49]

shess@chromium.org changed reviewers:
+ pwnall@chromium.org

[Scott Hess - ex-Googler, 2017-04-27 20:22:50]

lgtm, but I'm no longer owner.

Victor, I think this is safe, because it's quite restricted.  Per the thread on
the bug, this does probably modestly limit the fuzzer's ability to reach certain
edge cases, but given how the fuzzer operates it's fairly likely that those
cases can't be reached in the lifetime of the universe anyhow, or at least in
the lifetime of any of us, so that is probably moot.

[pwnall, 2017-04-27 20:40:07]

I'm ok with the principle. Can you please explain the benefits in the CL and
perhaps expand the comment a bit?

Is the concern that we're running out of space on bots, or are we trying to
limit the search space to get more interesting results? It'd be nice to have
this recorded in the code and git history.

[Scott Hess - ex-Googler, 2017-04-27 20:55:20]

On 2017/04/27 20:40:07, pwnall wrote:
> I'm ok with the principle. Can you please explain the benefits in the CL and
> perhaps expand the comment a bit?
> 
> Is the concern that we're running out of space on bots, or are we trying to
> limit the search space to get more interesting results? It'd be nice to have
> this recorded in the code and git history.

The fuzzer has rules for constructing SQL statements to feed to SQLite, which in
this bug resulted in a huge in-memory database (see comment #4 on bug).  While
huge databases need fuzzing, too, fuzzing huge databases is an inefficient use
of resources (by the time you reach the external OOM, you could probably have
run dozens of smaller fuzzes).  So this CL modifies the SQLite the fuzzer uses
to restrict the footprint at the SQLite level.

IMHO this is fine, because it should still cover all of the interesting cases
like statement parsing and schema weirdness, which I believe is the primary goal
of this particular fuzzer.

[pwnall, 2017-04-27 21:00:27]

On 2017/04/27 20:55:20, Scott Hess - ex-Googler wrote:
> On 2017/04/27 20:40:07, pwnall wrote:
> > I'm ok with the principle. Can you please explain the benefits in the CL and
> > perhaps expand the comment a bit?
> > 
> > Is the concern that we're running out of space on bots, or are we trying to
> > limit the search space to get more interesting results? It'd be nice to have
> > this recorded in the code and git history.
> 
> The fuzzer has rules for constructing SQL statements to feed to SQLite, which
in
> this bug resulted in a huge in-memory database (see comment #4 on bug).  While
> huge databases need fuzzing, too, fuzzing huge databases is an inefficient use
> of resources (by the time you reach the external OOM, you could probably have
> run dozens of smaller fuzzes).  So this CL modifies the SQLite the fuzzer uses
> to restrict the footprint at the SQLite level.
> 
> IMHO this is fine, because it should still cover all of the interesting cases
> like statement parsing and schema weirdness, which I believe is the primary
goal
> of this particular fuzzer.

Sorry for being unclear. I agree with the content of the CL. I would like to
have the reasoning documented in BUILD.gn and in the CL so that if I look here 2
years from now I remember what happened  :)

If the writeup is too much hassle, LGTM to land as-is, and I'll add a comment
afterwards :)

[mmoroz, 2017-04-28 12:55:25]

On 2017/04/27 21:00:27, pwnall wrote:
> On 2017/04/27 20:55:20, Scott Hess - ex-Googler wrote:
> > On 2017/04/27 20:40:07, pwnall wrote:
> > > I'm ok with the principle. Can you please explain the benefits in the CL
and
> > > perhaps expand the comment a bit?
> > > 
> > > Is the concern that we're running out of space on bots, or are we trying
to
> > > limit the search space to get more interesting results? It'd be nice to
have
> > > this recorded in the code and git history.
> > 
> > The fuzzer has rules for constructing SQL statements to feed to SQLite,
which
> in
> > this bug resulted in a huge in-memory database (see comment #4 on bug). 
While
> > huge databases need fuzzing, too, fuzzing huge databases is an inefficient
use
> > of resources (by the time you reach the external OOM, you could probably
have
> > run dozens of smaller fuzzes).  So this CL modifies the SQLite the fuzzer
uses
> > to restrict the footprint at the SQLite level.
> > 
> > IMHO this is fine, because it should still cover all of the interesting
cases
> > like statement parsing and schema weirdness, which I believe is the primary
> goal
> > of this particular fuzzer.
> 
> Sorry for being unclear. I agree with the content of the CL. I would like to
> have the reasoning documented in BUILD.gn and in the CL so that if I look here
2
> years from now I remember what happened  :)
> 
> If the writeup is too much hassle, LGTM to land as-is, and I'll add a comment
> afterwards :)

Thanks for taking a look! I've updated the comment :)

[mmoroz, 2017-04-28 12:55:36]

The CQ bit was checked by mmoroz@chromium.org

[mmoroz, 2017-04-28 12:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org,
pwnall@chromium.org
Link to the patchset: https://codereview.chromium.org/2609473004/#ps60001
(title: "Expand the comment")

[commit-bot: I haz the power, 2017-04-28 12:56:02]

CQ is trying da patch.

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

[pwnall, 2017-04-28 13:36:38]

On 2017/04/28 12:55:25, mmoroz wrote:
> On 2017/04/27 21:00:27, pwnall wrote:
> > On 2017/04/27 20:55:20, Scott Hess - ex-Googler wrote:
> > > On 2017/04/27 20:40:07, pwnall wrote:
> > > > I'm ok with the principle. Can you please explain the benefits in the CL
> and
> > > > perhaps expand the comment a bit?
> > > > 
> > > > Is the concern that we're running out of space on bots, or are we trying
> to
> > > > limit the search space to get more interesting results? It'd be nice to
> have
> > > > this recorded in the code and git history.
> > > 
> > > The fuzzer has rules for constructing SQL statements to feed to SQLite,
> which
> > in
> > > this bug resulted in a huge in-memory database (see comment #4 on bug). 
> While
> > > huge databases need fuzzing, too, fuzzing huge databases is an inefficient
> use
> > > of resources (by the time you reach the external OOM, you could probably
> have
> > > run dozens of smaller fuzzes).  So this CL modifies the SQLite the fuzzer
> uses
> > > to restrict the footprint at the SQLite level.
> > > 
> > > IMHO this is fine, because it should still cover all of the interesting
> cases
> > > like statement parsing and schema weirdness, which I believe is the
primary
> > goal
> > > of this particular fuzzer.
> > 
> > Sorry for being unclear. I agree with the content of the CL. I would like to
> > have the reasoning documented in BUILD.gn and in the CL so that if I look
here
> 2
> > years from now I remember what happened  :)
> > 
> > If the writeup is too much hassle, LGTM to land as-is, and I'll add a
comment
> > afterwards :)
> 
> Thanks for taking a look! I've updated the comment :)

This is great! Thank you very much for the comment!

[commit-bot: I haz the power, 2017-04-28 14:10:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493384136947910,
"parent_rev": "824d3f765ad699054db51cc21c7a9deb8d17a947", "commit_rev":
"927053fa078035ee9bea31ef77184ccbe8001745"}

[commit-bot: I haz the power, 2017-04-28 14:10:34]

Description was changed from

==========
[sqlite3] Limit max number of memory pages for fuzzing builds.

R=shess@chromium.org
BUG=675446
==========

to

==========
[sqlite3] Limit max number of memory pages for fuzzing builds.

R=shess@chromium.org
BUG=675446

Review-Url: https://codereview.chromium.org/2609473004
Cr-Commit-Position: refs/heads/master@{#467984}
Committed:
https://chromium.googlesource.com/chromium/src/+/927053fa078035ee9bea31ef7718...
==========

[commit-bot: I haz the power, 2017-04-28 14:10:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/927053fa078035ee9bea31ef7718...