[sql] Prevent nChildren overrun decoding interior pages in recover.c.

[sql] Prevent nChildren overrun decoding interior pages in recover.c.

A corrupt header could result in attempting to read cells past the end
of the page.  Prevent this by capping nChildren based on the amount of
overhead cells require.

BUG=387868
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280047

[Scott Hess - ex-Googler, 2014-06-25 19:56:03]

I wrote a unit test to force this situation to happen, but was unable to
reproduce the crash, probably because of differences in memory allocators.  I
was able to see the buffer overrun, though, and this fixed it, so I _think_ it's
good.

The math is based on:
  http://www.sqlite.org/fileformat2.html
section "1.5 B-tree Pages".  The table interior cell content is described in
"Table B-Tree Interior Cell" under that.

[Scott Hess - ex-Googler, 2014-06-25 20:19:41]

On 2014/06/25 19:56:03, shess wrote:
> I wrote a unit test to force this situation to happen, but was unable to
> reproduce the crash, probably because of differences in memory allocators.  I
> was able to see the buffer overrun, though, and this fixed it, so I _think_
it's
> good.

It occurred to me to try with asan, and it repro'ed where expected, so I don't
think the caveat stands.

[michaeln, 2014-06-26 01:25:32]

lgtm

> It occurred to me to try with asan, and it repro'ed where expected, so I don't
> think the caveat stands.
would it make sense to check the test in too?

[Scott Hess - ex-Googler, 2014-06-26 15:59:35]

On 2014/06/26 01:25:32, michaeln wrote:
> lgtm
> 
> > It occurred to me to try with asan, and it repro'ed where expected, so I
don't
> > think the caveat stands.
> would it make sense to check the test in too?

I'm really mixed about it.  Since it has to muck with the raw file format, it's
pretty specific to the exact line of code involved, and feels pretty brittle WRT
SQLite changes.

How about I'll land this and post the unit test and we can discuss from there.

[Scott Hess - ex-Googler, 2014-06-26 15:59:56]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2014-06-26 16:00:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shess@chromium.org/343423004/1

[Scott Hess - ex-Googler, 2014-06-26 17:11:07]

On 2014/06/26 15:59:35, shess wrote:
> On 2014/06/26 01:25:32, michaeln wrote:
> > lgtm
> > 
> > > It occurred to me to try with asan, and it repro'ed where expected, so I
> don't
> > > think the caveat stands.
> > would it make sense to check the test in too?
> 
> I'm really mixed about it.  Since it has to muck with the raw file format,
it's
> pretty specific to the exact line of code involved, and feels pretty brittle
WRT
> SQLite changes.
> 
> How about I'll land this and post the unit test and we can discuss from there.

I just thought of a better way.  The current test works like "Use SQLite API to
generate a database with certain characteristics, then manually modify the
underlying file to provoke the case."  So it's pretty dependent on SQLite
internals.  But if I just grab the database generated and store it in a golden
file, then it's reasonable to expect that file to work correctly with future
changes to SQLite.  I'll write that up and post it to you.

[commit-bot: I haz the power, 2014-06-26 17:58:28]

Change committed as 280047