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

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

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)

Comment 1

[Brett Wilson]

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.

Comment 2

[georgi - hopefully not receiving bugspam]

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.

Comment 3

[georgi - hopefully not receiving bugspam]

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.

Comment 4

[Stefan Sitter [:ssitter]]

(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?

Comment 5

[georgi - hopefully not receiving bugspam]

(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.

 

Comment 6

[georgi - hopefully not receiving bugspam]

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?

Comment 7

[georgi - hopefully not receiving bugspam]

Attachment: patch from: http://www.sqlite.org/cvstrac/chngview?cn=3174

patch from: http://www.sqlite.org/cvstrac/chngview?cn=3174

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[Mike Shaver (:shaver emeritus)]

Vlad says Brett does the mergers from upstream these days, reassigning optimistically.

Comment 10

[Brett Wilson]

I'll do it after safe browsing is in good shape.

Comment 11

[Brett Wilson]

This is fixed by the upgrade to sqlite 3.3.5 in bug 338155.

Comment 12

[Brett Wilson]

Fixed by patch in bug 338155.