442668 - gcc optimization trigger data corruption in sqlite3

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Sergey Yanovich]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008061513 Firefox/3.0
Build Identifier: 3.0 branch after sqlite 3.5.9

Mozilla's version of libsqlite3.so is failing the attached test case.

Reproducible: Always

Steps to Reproduce:
1. Place attached tkt3194.c & tkt3194.sqlite into db/sqlite3/src
2.  gcc -L $OBJDIR/dist/bin -lsqlite3 -Wl,-rpath,$OBJDIR/dist/bin -o tkt3194 tkt3194.c
3. ./tkt3194 tkt3194.sqlite

Actual Results:  
tkt3194.sqlite
param count: 3
column count: 7
is expired: 0
result: 0.000000


Expected Results:  
tkt3194.sqlite
param count: 3
column count: 7
is expired: 0
result: 48000.000000


This only happens with Mozilla's version. Both system (Debian/unstable) and sqlite3 default builds are working normally.

Comment 1

[Sergey Yanovich]

Attachment: test case database

Comment 2

[Sergey Yanovich]

Attachment: test case program

Comment 3

[Shawn Wilsher :sdwilsh]

We'll wait for sqlite to fix this bug.  Our configuration is in fact supported by the sqlite folks.

Comment 4

[Sergey Yanovich]

Attachment: proposed fix

The bug appears when libsqlite3.so is compiled by late gcc-4.2 or gcc-4.3 compilers. The original code is using an unreliable isnan() check, which fails when its argument is preserved in FPU after a previous operation.

This patch ensures data round-trip to RAM, so that any extra bits are truncated.

This patch is made against 3.5.9 and valid for 1.9.0.x branch. Trunk may or may not need a similar correction.

Comment 5

[Shawn Wilsher :sdwilsh]

Our normal approach here is to get a new build from the sqlite folks.  Did this fix make 3.6.0?

Comment 6

[D. Richard Hipp]

Yes.  The suggested patch from Sergey is in SQLite 3.6.0.

Comment 7

[Sergey Yanovich]

It is landed in sqlite check-in 5396. The bug can result in severe data corruption (see sqlite ticket 3202 for more details), so an exception may make sense for this bug.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 329840 [details] [diff] [review]
proposed fix

we'll get this fixed when we upgrade then

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> It is landed in sqlite check-in 5396. The bug can result in severe data
> corruption (see sqlite ticket 3202 for more details), so an exception may make
> sense for this bug.
Drivers have been pretty clear about only taking versions of sqlite from the sqlite folks.  We may need to get a custom build for 1.9.0, but I suspect we may backport 3.6.0 if it looks good.

Comment 10

[Sergey Yanovich]

Either way it is going to be fixed (by taking attachment 329840 [details] [diff] [review] or by upgrading to 3.6.0), this is probably worth to be shipped in 1.9.0.2, so nominating as blocker.

As a side note, 3.6.0 is going to prohibit gcc floating point optimization (-ffast-math), so it has a chance to be declined on performance reasons as 3.5.9 was.

Comment 11

[Samuel Sidler (old account; do not CC)]

Not blocking unless I hear a good reason why from sdwilsh.

Comment 12

[Sergey Yanovich]

(In reply to comment #11)
> Not blocking unless I hear a good reason why from sdwilsh.

As things stand now, if any floating point operation inside sqlite should produce a result that is not a whole number, it produces NULL instead.

Isn't this a *perfect* reason?!

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> As things stand now, if any floating point operation inside sqlite should
> produce a result that is not a whole number, it produces NULL instead.
> 
> Isn't this a *perfect* reason?!
Demonstrate real situations that come up frequently for this, and maybe.  Until then, that argument alone is not going to convince drivers to take this on a stability branch.

Comment 14

[Sergey Yanovich]

(In reply to comment #13)
> Demonstrate real situations that come up frequently for this, and maybe.

I am not sure how sqlite is used behind the Mozilla hood. Of what I've heard, awesomebar fresency could be such a situation, if it is calculated inside sqlite and can be a non-whole number.

And any extension using mozIStorage* family is also affected.

Comment 15

[Shawn Wilsher :sdwilsh]

Fixed by the sqlite upgrade on mozilla-central.  I've nominated that for branch as well.

Comment 16

[Shawn Wilsher :sdwilsh]

nevermind - sqlite got backed out

Comment 17

[Shawn Wilsher :sdwilsh]

sqlite upgrade stuck this time.  Resolving as FIXED.