Fix a crasher in full text search (sqlite)...

Fix a crasher in full text search (sqlite)
- If the xxx_segdir table gets corrupted, you can have non-contiguous indexes (idx).
- This causes an assertion in debug, and a crash later on on release

With this change it will return 'corrupted db'

We shall wait to get a couple more fixes to upstream to sqlite org.

BUG=21377
TEST=see bug

[Scott Hess - ex-Googler, 2009-09-12 16:28:04]

Looks Great To Me.

[cpu_(ooo_6.6-7.5), 2009-09-14 17:38:41]

Committed but gcl failed to close the issue. Grrr.

Presubmit checks took 2.0s to calculate.
Loaded authentication cookies from C:\Users\cpu\.codereview_upload_cookies
Sending        third_party\sqlite\ext\fts2\fts2.c
Transmitting file data .
Committed revision 26118.
Loaded authentication cookies from C:\Users\cpu\.codereview_upload_cookies
Error accessing url /203046/close




On 2009/09/12 16:28:04, shess wrote:
> Looks Great To Me.

[Chris Evans, 2009-09-16 00:22:29]

Sorry to be late to the party!
Can you also return a corruption error if "i >= MERGE_COUNT"? This would replace
the assert() of the same.

Currently, the code could cause a stack-based buffer overflow. I do admit, a
corruption that _adds_ a row with the correct out-out-bounds index is unlikely
but we should fix it too if we are changing this loop.

On 2009/09/14 17:38:41, cpu wrote:
> Committed but gcl failed to close the issue. Grrr.
> 
> Presubmit checks took 2.0s to calculate.
> Loaded authentication cookies from C:\Users\cpu\.codereview_upload_cookies
> Sending        third_party\sqlite\ext\fts2\fts2.c
> Transmitting file data .
> Committed revision 26118.
> Loaded authentication cookies from C:\Users\cpu\.codereview_upload_cookies
> Error accessing url /203046/close
> 
> 
> 
> 
> On 2009/09/12 16:28:04, shess wrote:
> > Looks Great To Me.

[Scott Hess - ex-Googler, 2009-09-16 00:29:42]

Seems reasonable.  Adding the right index and the right data types
would be pretty amazing, but checkout against a constant is probably
cheap, too.

BTW, I forgot to nit on style.  "i!=3DiIndex )" (no space around
operator, space before closing paren).  Sorry.  SQLite's style is very
close to the exact opposite of Google3 style :-).

-scott


On Tue, Sep 15, 2009 at 5:22 PM,  <cevans@chromium.org> wrote:
> Sorry to be late to the party!
> Can you also return a corruption error if "i >=3D MERGE_COUNT"? This woul=
d
> replace
> the assert() of the same.
>
> Currently, the code could cause a stack-based buffer overflow. I do admit=
, a
> corruption that _adds_ a row with the correct out-out-bounds index is
> unlikely
> but we should fix it too if we are changing this loop.
>
> On 2009/09/14 17:38:41, cpu wrote:
>>
>> Committed but gcl failed to close the issue. Grrr.
>
>> Presubmit checks took 2.0s to calculate.
>> Loaded authentication cookies from C:\Users\cpu\.codereview_upload_cooki=
es
>> Sending =A0 =A0 =A0 =A0third_party\sqlite\ext\fts2\fts2.c
>> Transmitting file data .
>> Committed revision 26118.
>> Loaded authentication cookies from C:\Users\cpu\.codereview_upload_cooki=
es
>> Error accessing url /203046/close
>
>
>
>
>> On 2009/09/12 16:28:04, shess wrote:
>> > Looks Great To Me.
>
>
>
> http://codereview.chromium.org/203046
>

[Chris Evans, 2009-09-16 07:26:41]

One more thing. I think the fix is incomplete in that if there are missing
entries at the end of a contiguous run, we'll still crash. e.g

0,1,2,3 <rest missing up to 15> will pass the new check but still leave NULL
structures to crash later.

I'll look into a CL.

On 2009/09/16 00:29:42, shess wrote:
> Seems reasonable.  Adding the right index and the right data types
> would be pretty amazing, but checkout against a constant is probably
> cheap, too.
> 
> BTW, I forgot to nit on style.  "i!=3DiIndex )" (no space around
> operator, space before closing paren).  Sorry.  SQLite's style is very
> close to the exact opposite of Google3 style :-).
> 
> -scott
> 
> 
> On Tue, Sep 15, 2009 at 5:22 PM,  <mailto:cevans@chromium.org> wrote:
> > Sorry to be late to the party!
> > Can you also return a corruption error if "i >=3D MERGE_COUNT"? This woul=
> d
> > replace
> > the assert() of the same.
> >
> > Currently, the code could cause a stack-based buffer overflow. I do admit=
> , a
> > corruption that _adds_ a row with the correct out-out-bounds index is
> > unlikely
> > but we should fix it too if we are changing this loop.
> >
> > On 2009/09/14 17:38:41, cpu wrote:
> >>
> >> Committed but gcl failed to close the issue. Grrr.
> >
> >> Presubmit checks took 2.0s to calculate.
> >> Loaded authentication cookies from C:\Users\cpu\.codereview_upload_cooki=
> es
> >> Sending =A0 =A0 =A0 =A0third_party\sqlite\ext\fts2\fts2.c
> >> Transmitting file data .
> >> Committed revision 26118.
> >> Loaded authentication cookies from C:\Users\cpu\.codereview_upload_cooki=
> es
> >> Error accessing url /203046/close
> >
> >
> >
> >
> >> On 2009/09/12 16:28:04, shess wrote:
> >> > Looks Great To Me.
> >
> >
> >
> > http://codereview.chromium.org/203046
> >

[Scott Hess - ex-Googler, 2009-09-16 17:32:49]

Chris,

I'll give it a once-over to see if there are similar opportunities.
By "it" I mean "fts2.c", not just this occurance.  In general the
places where the code does queries can easily return SQLite codes,
it's the deeper data-structure manipulation stuff which is harder to
armor against corruption (because you have to revise many callers to
propagate codes).

-scott


On Wed, Sep 16, 2009 at 12:26 AM,  <cevans@chromium.org> wrote:
> One more thing. I think the fix is incomplete in that if there are missin=
g
> entries at the end of a contiguous run, we'll still crash. e.g
>
> 0,1,2,3 <rest missing up to 15> will pass the new check but still leave N=
ULL
> structures to crash later.
>
> I'll look into a CL.
>
> On 2009/09/16 00:29:42, shess wrote:
>>
>> Seems reasonable. =A0Adding the right index and the right data types
>> would be pretty amazing, but checkout against a constant is probably
>> cheap, too.
>
>> BTW, I forgot to nit on style. =A0"i!=3D3DiIndex )" (no space around
>> operator, space before closing paren). =A0Sorry. =A0SQLite's style is ve=
ry
>> close to the exact opposite of Google3 style :-).
>
>> -scott
>
>
>> On Tue, Sep 15, 2009 at 5:22 PM, =A0<mailto:cevans@chromium.org> wrote:
>> > Sorry to be late to the party!
>> > Can you also return a corruption error if "i >=3D3D MERGE_COUNT"? This
>> > woul=3D
>> d
>> > replace
>> > the assert() of the same.
>> >
>> > Currently, the code could cause a stack-based buffer overflow. I do
>> > admit=3D
>> , a
>> > corruption that _adds_ a row with the correct out-out-bounds index is
>> > unlikely
>> > but we should fix it too if we are changing this loop.
>> >
>> > On 2009/09/14 17:38:41, cpu wrote:
>> >>
>> >> Committed but gcl failed to close the issue. Grrr.
>> >
>> >> Presubmit checks took 2.0s to calculate.
>> >> Loaded authentication cookies from
>> >> C:\Users\cpu\.codereview_upload_cooki=3D
>> es
>> >> Sending =3DA0 =3DA0 =3DA0 =3DA0third_party\sqlite\ext\fts2\fts2.c
>> >> Transmitting file data .
>> >> Committed revision 26118.
>> >> Loaded authentication cookies from
>> >> C:\Users\cpu\.codereview_upload_cooki=3D
>> es
>> >> Error accessing url /203046/close
>> >
>> >
>> >
>> >
>> >> On 2009/09/12 16:28:04, shess wrote:
>> >> > Looks Great To Me.
>> >
>> >
>> >
>> > http://codereview.chromium.org/203046
>> >
>
>
>
> http://codereview.chromium.org/203046
>

[tony, 2009-09-23 21:45:30]

Late drive-by-nit:
Can you create a patch file in the sqlite directory update README.chromium? 
Also, can you upstream the patch?

[cevans, 2009-09-23 21:52:16]

I'll take care of creating a "fts2-fixes.patch" which will be cpu@'s plus my
tweak plus my other megapatch. I will also offer it upstream.

On Wed, Sep 23, 2009 at 2:45 PM, <tony@chromium.org> wrote:

> Late drive-by-nit:
> Can you create a patch file in the sqlite directory update README.chromium?
> Also, can you upstream the patch?
>
>
> http://codereview.chromium.org/203046
>