Fix issue 15261: Crash in history::TextDatabase::GetTextMatches...

Fix issue 15261: Crash in history::TextDatabase::GetTextMatches

The crash in history::TextDatabase::GetTextMatches is caused by unexpected result of tolower() in some locales such as tr_TR.UTF-8. This CL fixes this issue by using following statement to replace tolower():

(ch>='A' && ch<='Z') ? (ch-'A'+'a') : ch

which will always return expected result for ascii characters regardless of current locale.

BUG=15261
Crash in history::TextDatabase::GetTextMatches
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25141

[James Su, 2009-08-25 04:37:20]

A CL to fix issue 15261. Please help review.

Regards
James Su

[Evan Martin, 2009-08-25 17:54:35]

Our changes to sqlite are not tracked in a very useful way :( but if you read
README.chromium there are notes about what we've done.

I think you should do this as:
1) save the patch you wrote here (I'll review it in a sec), then upload it to
sqlite upstream
2) copy the .patch file into our sqlite tree and update the README to reference
the .patch file
3) then make a new review (or add to this review) both the code change and the
README/.patch file for checkin

[Evan Martin, 2009-08-25 17:55:18]

http://codereview.chromium.org/174387/diff/1/6
File third_party/sqlite/ext/fts1/fts1.c (right):

http://codereview.chromium.org/174387/diff/1/6#newcode211
Line 211: return (c >= 'A' && c<= 'Z') ? (c + 0x20) : c;
try to match the sqlite style for spacing (look at the surrounding code for
inspiration

[Scott Hess - ex-Googler, 2009-08-25 21:20:48]

Elaborating on the SQLite upstreaming, you should verify that the problem still
exists in their current code, log a bug in their tracker which describes how to
replicate it (preferably on Linux), and attach the patch to the bug.  Even
better if you can provide a patch to add a test to exercise the bug and fix.  I
think you're fine to use your chromium.org account if it needs an email.

I know that's annoying, but I don't think anyone local is better placed to ferry
it up.  I've not pushed or pulled upstream for maybe a year or so, and can't
think of anyone who has done anything material in over a quarter.  That said,
SQLite is pretty easy to do such things with.

http://codereview.chromium.org/174387/diff/1/6
File third_party/sqlite/ext/fts1/fts1.c (right):

http://codereview.chromium.org/174387/diff/1/6#newcode211
Line 211: return (c >= 'A' && c<= 'Z') ? (c + 0x20) : c;
+1 to Evan's style point.  SQLite style is kinda opposite of google3 :-).

Suggest (c-'A'+'a') rather than using a magic constant.

http://codereview.chromium.org/174387/diff/1/4
File third_party/sqlite/ext/fts1/simple_tokenizer.c (right):

http://codereview.chromium.org/174387/diff/1/4#newcode142
Line 142: (unsigned char)((ch >= 'A' && ch <= 'Z') ? (ch + 0x20) : ch);
I could be mistaken, but I think the previous code read as:
  ((unsigned char)ch)<0x80 ...
and not:
  (unsigned char)(ch<0x80 ...)

In which case the new comparison can dispense with the cast entirely (like the
other occurences).

[James Su, 2009-08-26 01:36:53]

On 2009/08/25 17:54:35, Evan Martin wrote:
> Our changes to sqlite are not tracked in a very useful way :( but if you read
> README.chromium there are notes about what we've done.
> 
> I think you should do this as:
> 1) save the patch you wrote here (I'll review it in a sec), then upload it to
> sqlite upstream
> 2) copy the .patch file into our sqlite tree and update the README to
reference
> the .patch file
Which README file should I touch? I found that there is a preload-cache.patch
file but none of README and README.chromium reference this patch.

> 3) then make a new review (or add to this review) both the code change and the
> README/.patch file for checkin

[James Su, 2009-08-26 03:09:32]

Hi,
  I just checked the source code of the latest sqlite release 3.6.17 and its cvs
head, and found that the tolower/toupper issue in core sqlite library has been
fixed by introducing following macros in sqliteInt.h:

...
/*
** Check to see if this machine uses EBCDIC.  (Yes, believe it or
** not, there are still machines out there that use EBCDIC.)
*/
#if 'A' == '\301'
# define SQLITE_EBCDIC 1
#else
# define SQLITE_ASCII 1
#endif

...

/*
** The following macros mimic the standard library functions toupper(),
** isspace(), isalnum(), isdigit() and isxdigit(), respectively. The
** sqlite versions only work for ASCII characters, regardless of locale.
*/
#ifdef SQLITE_ASCII
# define sqlite3Toupper(x)  ((x)&~(sqlite3CtypeMap[(unsigned char)(x)]&0x20))
# define sqlite3Isspace(x)   (sqlite3CtypeMap[(unsigned char)(x)]&0x01)
# define sqlite3Isalnum(x)   (sqlite3CtypeMap[(unsigned char)(x)]&0x06)
# define sqlite3Isalpha(x)   (sqlite3CtypeMap[(unsigned char)(x)]&0x02)
# define sqlite3Isdigit(x)   (sqlite3CtypeMap[(unsigned char)(x)]&0x04)
# define sqlite3Isxdigit(x)  (sqlite3CtypeMap[(unsigned char)(x)]&0x08)
# define sqlite3Tolower(x)   (sqlite3UpperToLower[(unsigned char)(x)])
#else
# define sqlite3Toupper(x)   toupper((unsigned char)(x))
# define sqlite3Isspace(x)   isspace((unsigned char)(x))
# define sqlite3Isalnum(x)   isalnum((unsigned char)(x))
# define sqlite3Isalpha(x)   isalpha((unsigned char)(x))
# define sqlite3Isdigit(x)   isdigit((unsigned char)(x))
# define sqlite3Isxdigit(x)  isxdigit((unsigned char)(x))
# define sqlite3Tolower(x)   tolower((unsigned char)(x))
#endif

See: http://www.sqlite.org/cvstrac/chngview?cn=6196 for details. But
unfortunately the code in ext/fts? were not fixed yet.

So I think it's better for us to sync to the latest sqlite version before
applying this CL.

Regards
James Su

On 2009/08/26 01:36:53, James Su wrote:
> On 2009/08/25 17:54:35, Evan Martin wrote:
> > Our changes to sqlite are not tracked in a very useful way :( but if you
read
> > README.chromium there are notes about what we've done.
> > 
> > I think you should do this as:
> > 1) save the patch you wrote here (I'll review it in a sec), then upload it
to
> > sqlite upstream
> > 2) copy the .patch file into our sqlite tree and update the README to
> reference
> > the .patch file
> Which README file should I touch? I found that there is a preload-cache.patch
> file but none of README and README.chromium reference this patch.
> 
> > 3) then make a new review (or add to this review) both the code change and
the
> > README/.patch file for checkin

[James Su, 2009-08-26 03:14:51]

CL updated according to your comments, except for the README change.

Regards
James Su

http://codereview.chromium.org/174387/diff/1/6
File third_party/sqlite/ext/fts1/fts1.c (right):

http://codereview.chromium.org/174387/diff/1/6#newcode211
Line 211: return (c >= 'A' && c<= 'Z') ? (c + 0x20) : c;
On 2009/08/25 21:20:49, shess wrote:
> +1 to Evan's style point.  SQLite style is kinda opposite of google3 :-).
> 
> Suggest (c-'A'+'a') rather than using a magic constant.

Done.

http://codereview.chromium.org/174387/diff/1/4
File third_party/sqlite/ext/fts1/simple_tokenizer.c (right):

http://codereview.chromium.org/174387/diff/1/4#newcode142
Line 142: (unsigned char)((ch >= 'A' && ch <= 'Z') ? (ch + 0x20) : ch);
On 2009/08/25 21:20:49, shess wrote:
> I could be mistaken, but I think the previous code read as:
>   ((unsigned char)ch)<0x80 ...
> and not:
>   (unsigned char)(ch<0x80 ...)
> 
> In which case the new comparison can dispense with the cast entirely (like the
> other occurences).

Done.

[Scott Hess - ex-Googler, 2009-08-26 03:17:11]

I think we should upstream the fix so that when we sync with latest
SQLite we don't have to fix it again (or forget to fix it - it might
be worthwhile to have a history unit test somewhere which munges the
locale to make sure the fix is not lost).

As far as sync'ing with latest SQLite version ... we'd need someone
willing to review whether there have been changes we need to worry
about since the last sync, especially WRT SQLite access from
JavaScript.  Since the last sync was like a year ago, this is sort of
a project, it would probably be more expedient to document and apply
our patch locally.  Anyone disagree?

-scott


On Tue, Aug 25, 2009 at 8:09 PM, <suzhe@chromium.org> wrote:
> Hi,
> =A0I just checked the source code of the latest sqlite release 3.6.17 and
> its cvs head, and found that the tolower/toupper issue in core sqlite
> library has been fixed by introducing following macros in sqliteInt.h:
>
> ...
> /*
> ** Check to see if this machine uses EBCDIC. =A0(Yes, believe it or
> ** not, there are still machines out there that use EBCDIC.)
> */
> #if 'A' =3D=3D '\301'
> # define SQLITE_EBCDIC 1
> #else
> # define SQLITE_ASCII 1
> #endif
>
> ...
>
> /*
> ** The following macros mimic the standard library functions toupper(),
> ** isspace(), isalnum(), isdigit() and isxdigit(), respectively. The
> ** sqlite versions only work for ASCII characters, regardless of locale.
> */
> #ifdef SQLITE_ASCII
> # define sqlite3Toupper(x) =A0((x)&~(sqlite3CtypeMap[(unsigned
> char)(x)]&0x20))
> # define sqlite3Isspace(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x01)
> # define sqlite3Isalnum(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x06)
> # define sqlite3Isalpha(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x02)
> # define sqlite3Isdigit(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x04)
> # define sqlite3Isxdigit(x) =A0(sqlite3CtypeMap[(unsigned char)(x)]&0x08)
> # define sqlite3Tolower(x) =A0 (sqlite3UpperToLower[(unsigned char)(x)])
> #else
> # define sqlite3Toupper(x) =A0 toupper((unsigned char)(x))
> # define sqlite3Isspace(x) =A0 isspace((unsigned char)(x))
> # define sqlite3Isalnum(x) =A0 isalnum((unsigned char)(x))
> # define sqlite3Isalpha(x) =A0 isalpha((unsigned char)(x))
> # define sqlite3Isdigit(x) =A0 isdigit((unsigned char)(x))
> # define sqlite3Isxdigit(x) =A0isxdigit((unsigned char)(x))
> # define sqlite3Tolower(x) =A0 tolower((unsigned char)(x))
> #endif
>
> See: http://www.sqlite.org/cvstrac/chngview?cn=3D6196 for details. But
> unfortunately the code in ext/fts? were not fixed yet.
>
> So I think it's better for us to sync to the latest sqlite version
> before applying this CL.
>
> Regards
> James Su
>
> On 2009/08/26 01:36:53, James Su wrote:
>>
>> On 2009/08/25 17:54:35, Evan Martin wrote:
>> > Our changes to sqlite are not tracked in a very useful way :( but if
>
> you read
>>
>> > README.chromium there are notes about what we've done.
>> >
>> > I think you should do this as:
>> > 1) save the patch you wrote here (I'll review it in a sec), then
>
> upload it to
>>
>> > sqlite upstream
>> > 2) copy the .patch file into our sqlite tree and update the README
>
> to
>>
>> reference
>> > the .patch file
>> Which README file should I touch? I found that there is a
>
> preload-cache.patch
>>
>> file but none of README and README.chromium reference this patch.
>
>> > 3) then make a new review (or add to this review) both the code
>
> change and the
>>
>> > README/.patch file for checkin
>
>
>
> http://codereview.chromium.org/174387
>

[James Su, 2009-08-26 03:43:17]

A ticket has been created upstream:
http://www.sqlite.org/src/tktview/991789d9f3136a0460dc83a33e815c1aa9757c26

On 2009/08/26 03:17:11, shess wrote:
> I think we should upstream the fix so that when we sync with latest
> SQLite we don't have to fix it again (or forget to fix it - it might
> be worthwhile to have a history unit test somewhere which munges the
> locale to make sure the fix is not lost).
> 
> As far as sync'ing with latest SQLite version ... we'd need someone
> willing to review whether there have been changes we need to worry
> about since the last sync, especially WRT SQLite access from
> JavaScript.  Since the last sync was like a year ago, this is sort of
> a project, it would probably be more expedient to document and apply
> our patch locally.  Anyone disagree?
> 
> -scott
> 
> 
> On Tue, Aug 25, 2009 at 8:09 PM, <mailto:suzhe@chromium.org> wrote:
> > Hi,
> > =A0I just checked the source code of the latest sqlite release 3.6.17 and
> > its cvs head, and found that the tolower/toupper issue in core sqlite
> > library has been fixed by introducing following macros in sqliteInt.h:
> >
> > ...
> > /*
> > ** Check to see if this machine uses EBCDIC. =A0(Yes, believe it or
> > ** not, there are still machines out there that use EBCDIC.)
> > */
> > #if 'A' =3D=3D '\301'
> > # define SQLITE_EBCDIC 1
> > #else
> > # define SQLITE_ASCII 1
> > #endif
> >
> > ...
> >
> > /*
> > ** The following macros mimic the standard library functions toupper(),
> > ** isspace(), isalnum(), isdigit() and isxdigit(), respectively. The
> > ** sqlite versions only work for ASCII characters, regardless of locale.
> > */
> > #ifdef SQLITE_ASCII
> > # define sqlite3Toupper(x) =A0((x)&~(sqlite3CtypeMap[(unsigned
> > char)(x)]&0x20))
> > # define sqlite3Isspace(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x01)
> > # define sqlite3Isalnum(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x06)
> > # define sqlite3Isalpha(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x02)
> > # define sqlite3Isdigit(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x04)
> > # define sqlite3Isxdigit(x) =A0(sqlite3CtypeMap[(unsigned char)(x)]&0x08)
> > # define sqlite3Tolower(x) =A0 (sqlite3UpperToLower[(unsigned char)(x)])
> > #else
> > # define sqlite3Toupper(x) =A0 toupper((unsigned char)(x))
> > # define sqlite3Isspace(x) =A0 isspace((unsigned char)(x))
> > # define sqlite3Isalnum(x) =A0 isalnum((unsigned char)(x))
> > # define sqlite3Isalpha(x) =A0 isalpha((unsigned char)(x))
> > # define sqlite3Isdigit(x) =A0 isdigit((unsigned char)(x))
> > # define sqlite3Isxdigit(x) =A0isxdigit((unsigned char)(x))
> > # define sqlite3Tolower(x) =A0 tolower((unsigned char)(x))
> > #endif
> >
> > See: http://www.sqlite.org/cvstrac/chngview?cn=3D6196 for details. But
> > unfortunately the code in ext/fts? were not fixed yet.
> >
> > So I think it's better for us to sync to the latest sqlite version
> > before applying this CL.
> >
> > Regards
> > James Su
> >
> > On 2009/08/26 01:36:53, James Su wrote:
> >>
> >> On 2009/08/25 17:54:35, Evan Martin wrote:
> >> > Our changes to sqlite are not tracked in a very useful way :( but if
> >
> > you read
> >>
> >> > README.chromium there are notes about what we've done.
> >> >
> >> > I think you should do this as:
> >> > 1) save the patch you wrote here (I'll review it in a sec), then
> >
> > upload it to
> >>
> >> > sqlite upstream
> >> > 2) copy the .patch file into our sqlite tree and update the README
> >
> > to
> >>
> >> reference
> >> > the .patch file
> >> Which README file should I touch? I found that there is a
> >
> > preload-cache.patch
> >>
> >> file but none of README and README.chromium reference this patch.
> >
> >> > 3) then make a new review (or add to this review) both the code
> >
> > change and the
> >>
> >> > README/.patch file for checkin
> >
> >
> >
> > http://codereview.chromium.org/174387
> >

[James Su, 2009-09-01 06:47:41]

Ping.

On 2009/08/26 03:43:17, James Su wrote:
> A ticket has been created upstream:
> http://www.sqlite.org/src/tktview/991789d9f3136a0460dc83a33e815c1aa9757c26
> 
> On 2009/08/26 03:17:11, shess wrote:
> > I think we should upstream the fix so that when we sync with latest
> > SQLite we don't have to fix it again (or forget to fix it - it might
> > be worthwhile to have a history unit test somewhere which munges the
> > locale to make sure the fix is not lost).
> > 
> > As far as sync'ing with latest SQLite version ... we'd need someone
> > willing to review whether there have been changes we need to worry
> > about since the last sync, especially WRT SQLite access from
> > JavaScript.  Since the last sync was like a year ago, this is sort of
> > a project, it would probably be more expedient to document and apply
> > our patch locally.  Anyone disagree?
> > 
> > -scott
> > 
> > 
> > On Tue, Aug 25, 2009 at 8:09 PM, <mailto:suzhe@chromium.org> wrote:
> > > Hi,
> > > =A0I just checked the source code of the latest sqlite release 3.6.17 and
> > > its cvs head, and found that the tolower/toupper issue in core sqlite
> > > library has been fixed by introducing following macros in sqliteInt.h:
> > >
> > > ...
> > > /*
> > > ** Check to see if this machine uses EBCDIC. =A0(Yes, believe it or
> > > ** not, there are still machines out there that use EBCDIC.)
> > > */
> > > #if 'A' =3D=3D '\301'
> > > # define SQLITE_EBCDIC 1
> > > #else
> > > # define SQLITE_ASCII 1
> > > #endif
> > >
> > > ...
> > >
> > > /*
> > > ** The following macros mimic the standard library functions toupper(),
> > > ** isspace(), isalnum(), isdigit() and isxdigit(), respectively. The
> > > ** sqlite versions only work for ASCII characters, regardless of locale.
> > > */
> > > #ifdef SQLITE_ASCII
> > > # define sqlite3Toupper(x) =A0((x)&~(sqlite3CtypeMap[(unsigned
> > > char)(x)]&0x20))
> > > # define sqlite3Isspace(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x01)
> > > # define sqlite3Isalnum(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x06)
> > > # define sqlite3Isalpha(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x02)
> > > # define sqlite3Isdigit(x) =A0 (sqlite3CtypeMap[(unsigned char)(x)]&0x04)
> > > # define sqlite3Isxdigit(x) =A0(sqlite3CtypeMap[(unsigned char)(x)]&0x08)
> > > # define sqlite3Tolower(x) =A0 (sqlite3UpperToLower[(unsigned char)(x)])
> > > #else
> > > # define sqlite3Toupper(x) =A0 toupper((unsigned char)(x))
> > > # define sqlite3Isspace(x) =A0 isspace((unsigned char)(x))
> > > # define sqlite3Isalnum(x) =A0 isalnum((unsigned char)(x))
> > > # define sqlite3Isalpha(x) =A0 isalpha((unsigned char)(x))
> > > # define sqlite3Isdigit(x) =A0 isdigit((unsigned char)(x))
> > > # define sqlite3Isxdigit(x) =A0isxdigit((unsigned char)(x))
> > > # define sqlite3Tolower(x) =A0 tolower((unsigned char)(x))
> > > #endif
> > >
> > > See: http://www.sqlite.org/cvstrac/chngview?cn=3D6196 for details. But
> > > unfortunately the code in ext/fts? were not fixed yet.
> > >
> > > So I think it's better for us to sync to the latest sqlite version
> > > before applying this CL.
> > >
> > > Regards
> > > James Su
> > >
> > > On 2009/08/26 01:36:53, James Su wrote:
> > >>
> > >> On 2009/08/25 17:54:35, Evan Martin wrote:
> > >> > Our changes to sqlite are not tracked in a very useful way :( but if
> > >
> > > you read
> > >>
> > >> > README.chromium there are notes about what we've done.
> > >> >
> > >> > I think you should do this as:
> > >> > 1) save the patch you wrote here (I'll review it in a sec), then
> > >
> > > upload it to
> > >>
> > >> > sqlite upstream
> > >> > 2) copy the .patch file into our sqlite tree and update the README
> > >
> > > to
> > >>
> > >> reference
> > >> > the .patch file
> > >> Which README file should I touch? I found that there is a
> > >
> > > preload-cache.patch
> > >>
> > >> file but none of README and README.chromium reference this patch.
> > >
> > >> > 3) then make a new review (or add to this review) both the code
> > >
> > > change and the
> > >>
> > >> > README/.patch file for checkin
> > >
> > >
> > >
> > > http://codereview.chromium.org/174387
> > >

[Evan Martin, 2009-09-01 20:43:41]

LGTM.  Two suggestions for you to either add and commit or just commit without:

Can you update the README to mention the patch, if you think that's appropriate?

Can you add some text to the beginning of the patch that links to the bug or
describes what the patch works around?  (You can put arbitrary text at the
beginning of a .patch file)

[Scott Hess - ex-Googler, 2009-09-01 23:04:58]

LGTM.

+1 to Evan's comment about linking to the SQLite bug, so that when/if we ever
reimport that can easily be found.

On 2009/09/01 20:43:41, Evan Martin wrote:
> LGTM.  Two suggestions for you to either add and commit or just commit
without:
> 
> Can you update the README to mention the patch, if you think that's
appropriate?
> 
> Can you add some text to the beginning of the patch that links to the bug or
> describes what the patch works around?  (You can put arbitrary text at the
> beginning of a .patch file)