Closed Bug 334513 Opened 15 years ago Closed 15 years ago

probably 'off by a few' in sqlite3 utf.c

Categories

(Toolkit :: Storage, defect, P1)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta1

People

(Reporter: guninski, Assigned: brettw)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file)

probably off by one in sqlite3

on today's linux trunk under valgrind:

==27560== Invalid write of size 1
==27560==    at 0x1D359804: sqlite3VdbeMemTranslate (utf.c:355)
==27560==    by 0x1D3682FA: sqlite3VdbeChangeEncoding (vdbemem.c:49)
==27560==    by 0x1D369522: sqlite3ValueText (vdbemem.c:778)
==27560==    by 0x1D363806: sqlite3_value_text (vdbeapi.c:61)
==27560==    by 0x1D3643CF: sqlite3_column_text (vdbeapi.c:453)
==27560==    by 0x1D33C5D7: sqlite3_exec (legacy.c:92)
==27560==    by 0x1D34AA1B: sqlite3InitOne (prepare.c:296)
==27560==    by 0x1D34ABA0: sqlite3Init (prepare.c:338)
==27560==    by 0x1D34AC9C: sqlite3ReadSchema (prepare.c:375)
==27560==    by 0x1D328800: sqlite3LocateTable (build.c:284)
==27560==    by 0x1D3502BA: prepSelectStmt (select.c:1203)
==27560==    by 0x1D353275: sqlite3SelectResolve (select.c:2530)
==27560==  Address 0x1DFF74E7 is 0 bytes after a block of size 79 alloc'd
==27560==    at 0x11B1BE96: malloc (vg_replace_malloc.c:149)
==27560==    by 0x1D33E2BD: sqlite3GenericMalloc (os_common.h:174)
==27560==    by 0x1D35A04B: sqlite3MallocRaw (util.c:589)
==27560==    by 0x1D358F99: sqlite3VdbeMemTranslate (utf.c:314)
==27560==    by 0x1D3682FA: sqlite3VdbeChangeEncoding (vdbemem.c:49)
==27560==    by 0x1D369522: sqlite3ValueText (vdbemem.c:778)
==27560==    by 0x1D363806: sqlite3_value_text (vdbeapi.c:61)
==27560==    by 0x1D3643CF: sqlite3_column_text (vdbeapi.c:453)
==27560==    by 0x1D33C5D7: sqlite3_exec (legacy.c:92)
==27560==    by 0x1D34AA1B: sqlite3InitOne (prepare.c:296)
==27560==    by 0x1D34ABA0: sqlite3Init (prepare.c:338)
==27560==    by 0x1D34AC9C: sqlite3ReadSchema (prepare.c:375)
==27560== 
==27560== Invalid read of size 1
==27560==    at 0x1D355F64: sqlite3RunParser (tokenize.c:362)
==27560==    by 0x1D34B252: sqlite3_prepare (prepare.c:539)
==27560==    by 0x1D33C48D: sqlite3_exec (legacy.c:56)
Assignee: nobody → vladimir
Component: Startup and Profile System → Storage
Product: Firefox → Toolkit
QA Contact: startup → storage
I looked at the off by one problem and I don't think its an error. Perhaps somebody else can double-check:

321   if( pMem->enc==SQLITE_UTF8 ){
322     if( desiredEnc==SQLITE_UTF16LE ){
323       /* UTF-8 -> UTF-16 Little-endian */
324       while( zIn<zTerm ){
325         READ_UTF8(zIn, c); 
326         WRITE_UTF16LE(z, c);
327       }
328     }else{
329       assert( desiredEnc==SQLITE_UTF16BE );
330       /* UTF-8 -> UTF-16 Big-endian */
331       while( zIn<zTerm ){
332         READ_UTF8(zIn, c); 
333         WRITE_UTF16BE(z, c);
334       }
335     }
336     pMem->n = z - zOut;
337     *z++ = 0;
338   }else{
339     assert( desiredEnc==SQLITE_UTF8 );
340     if( pMem->enc==SQLITE_UTF16LE ){
341       /* UTF-16 Little-endian -> UTF-8 */
342       while( zIn<zTerm ){
343         READ_UTF16LE(zIn, c); 
344         WRITE_UTF8(z, c);
345       }
346     }else{
347       /* UTF-16 Little-endian -> UTF-8 */
348       while( zIn<zTerm ){
349         READ_UTF16BE(zIn, c); 
350         WRITE_UTF8(z, c);
351       }
352     }
353     pMem->n = z - zOut;
354   }
355   *z = 0;        <-------- warning

I suspect that we succeeded on the first if statement and went into the first block for converting from UTF-8. Line 337 does "*z++ = 0;", and then we do it again at the end on line 355. This is probably what's generating the warning. But we need both of these to write both terminating NULLs for the 16-bit characters.

Perhaps the warning was cause by somethign else, however.

I don't know about the other two errors. The alloc error sounds scary. I don't know what the third error means.
this sqlite3 thing seems quite f*cked.

according to the debugger, i land with:
len=79, desiredEnc==SQLITE_UTF8 and trying to write 0 one byte after an allocation of 79.

if this comment is to be believed:
---------------
 if( desiredEnc==SQLITE_UTF8 ){
    /* When converting from UTF-16, the maximum growth results from
    ** translating a 2-byte character to a 3-byte UTF-8 character (i.e.
    ** code-point 0xFFFC). A single byte is required for the output string
    ** nul-terminator.
    */
    len = (pMem->n/2) * 3 + 1;
---------------

pMem->n/2 is buggy for odd pMem->n ( div by 2 is rounded *down* for ints).

according to the source, an increment by more that 3 seems possible:
#define WRITE_UTF8(zOut, c) {
...
 }else{                                               \
    *zOut++ = 0xF0 + ((c>>18) & 0x07);                 \
    *zOut++ = 0x80 + ((c>>12) & 0x3F);                 \
    *zOut++ = 0x80 + ((c>>6) & 0x3F);                  \
    *zOut++ = 0x80 + (c & 0x3F); 

so i suggest the person who wrote this debug it a little.

Summary: probably 'off by one' in sqlite3 → probably 'off by a few' in sqlite3 utf.c
adding
#undef NDEBUG
just before:
#include <assert.h>

in utf.c
prevents firefox from starting because of this:
../db/sqlite3/src/utf.c:358: sqlite3VdbeMemTranslate: Assertion `(pMem->n+(desiredEnc==1?1:2))<=len' failed.
(In reply to comment #2)
I can't say if it is related but the code source you quoted was changed some days ago in sqlite. See http://www.sqlite.org/cvstrac/chngview?cn=3174 for details. Maybe the same problem?
(In reply to comment #4)
> I can't say if it is related but the code source you quoted was changed some
> days ago in sqlite. See http://www.sqlite.org/cvstrac/chngview?cn=3174 for
> details. Maybe the same problem?

yes, this seems reasonable patch to apply at first glance.

almost sure the same problem.

 

bz,

this is at least a real off by one and depending on heap layout this may crash on startup.

Comment #4 shows the solution from sqlite:
http://www.sqlite.org/cvstrac/chngview?cn=3174
Allocate enough memory for the worst-case UTF-16 to UTF-8 conversion. Ticket #1773
 
(the misleading comment was fixed).

can you help speeding fixing this?

Flags: blocking1.9a1?
Flags: blocking-firefox2?
This should get fixed on the 1.8 branch, of course.  Georgi was bugging because of lack of progress.  Vlad, is this well-assigned to you?

/be
Vlad says Brett does the mergers from upstream these days, reassigning optimistically.
Assignee: vladimir → brettw
I'll do it after safe browsing is in good shape.
Depends on: 338155
OS: Linux → All
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1beta1
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking-firefox2?
Flags: blocking-firefox2+
This is fixed by the upgrade to sqlite 3.3.5 in bug 338155.
Fixed by patch in bug 338155.
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.