Re: [sqlite] Uninitialized memory reads (not likely false positives)
On Wed, Nov 16, 2016 at 12:06:39PM -0600, Nico Williams wrote:
> On Tue, Nov 15, 2016 at 09:38:11PM -0200, Bernardo Sulzbach wrote:
> > if( s1>7 && s2>7 ){
> > res = s1 - s2;
> > }else{
> > if( s1==s2 ){
> > // Accesses to aLen as mentioned above
> >
> > If s1 > 7 && s2 > 7 is false, then at least one of s1 and s2 is not above 7.
> > If they are equal, then neither s1 nor s2 is above 7.
>
> 7 is past the end of the array.
>
> > > and we also know that either s1 or s2 can be 8 or 9,
> >
> > This is false, unless I am mistaken. See my reasoning above.
> >
> > The issue is valid, and the message your analyzer (or compiler) wrote is
> > correct: it is not guaranteed to be < 7, which it should be.
>
> Right.
Er, no, wrong: it has to be < 7 given the assertions and the if
conditions. I should have read Dan Kennedy's response first.
Nico
--
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Re: [sqlite] Uninitialized memory reads (not likely false positives)
On Wed, Nov 16, 2016 at 10:52:13PM +0700, Dan Kennedy wrote: > On 11/16/2016 05:53 AM, Nico Williams wrote: > > [...] > > > > Anyways, the analysis from here is non-trivial, and I can't convince > > myself that sNC.pNext will not be dereferenced. > > Thanks for taking the time to look into these. > > Some kind of assert() could be helpful there I think. The reason sNC.pNext > will not be accessed is that generateColumnNames() is only called (a) on a > top-level SELECT statement and (b) after all references have already > resolved successfully. Implying that this: > > http://www.sqlite.org/src/artifact/672b1af237ad2?ln=1406 > > is always true. That's... a bit too convoluted to for my liking. I'd rather have sNC.pNext initialized than an assertion that might get compiled out. This is a function invoked on statement preparation, so I think initializing sNC.pNext can't negatively affect performance in any terribly meaningful way. > > Another one that I find difficult to analyze is a possible > > out-of-bounds read in vdbeSorterCompareInt(): > > > > [...] > > > > At 85715 we know that (s1 <= 7 || s2 <= 7) && s1 == s2, and we also > > know that either s1 or s2 can be 8 or 9, so aLen[s1] at 85715 could > > very well have s1 > 6, which would read past the bounds of aLen[]. > > I think ( ( s1<=7 || s2<=7) && s1==s2 ) implies that s1<=7. And we assume > s1!=7 because there is an assert() that says so. Oh, I see it now. Yeah, OK. This is definitely a false positive. Thanks for the response, Nico -- ___ sqlite-users mailing list sqlite-users@mailinglists.sqlite.org http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Re: [sqlite] Uninitialized memory reads (not likely false positives)
On Tue, Nov 15, 2016 at 09:38:11PM -0200, Bernardo Sulzbach wrote:
> if( s1>7 && s2>7 ){
> res = s1 - s2;
> }else{
> if( s1==s2 ){
> // Accesses to aLen as mentioned above
>
> If s1 > 7 && s2 > 7 is false, then at least one of s1 and s2 is not above 7.
> If they are equal, then neither s1 nor s2 is above 7.
7 is past the end of the array.
> > and we also know that either s1 or s2 can be 8 or 9,
>
> This is false, unless I am mistaken. See my reasoning above.
>
> The issue is valid, and the message your analyzer (or compiler) wrote is
> correct: it is not guaranteed to be < 7, which it should be.
Right.
> I am unsure whether or not this is actually a bug, but it almost certainly
> is a mistake.
That's how it seems to me, yes.
Nico
--
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Re: [sqlite] Uninitialized memory reads (not likely false positives)
On 11/16/2016 05:53 AM, Nico Williams wrote:
I don't normally pay attention to warnings when compiling SQLite3, nor
to Coverity or other static analysis tools' output either, as I'm quite
aware that most of these are false positives and thus unwelcome noise
here.
However, I do sample them occasionally, and though usually such reports
are false positives, here are two that don't quite look like false
positives to me. I get these from building the SQLite3 3.15.1
amalgamation.
Uninitialized pointer dereference:
115861 static void generateColumnTypes(
115862 Parse *pParse, /* Parser context */
115863 SrcList *pTabList, /* List of tables */
115864 ExprList *pEList/* Expressions defining the result set */
115865 ){
115866 #ifndef SQLITE_OMIT_DECLTYPE
115867 Vdbe *v = pParse->pVdbe;
115868 int i;
1. var_decl: Declaring variable sNC without initializer.
115869 NameContext sNC;
115870 sNC.pSrcList = pTabList;
115871 sNC.pParse = pParse;
2. Condition i < pEList->nExpr, taking true branch
115872 for(i=0; inExpr; i++){
115873 Expr *p = pEList->a[i].pExpr;
115874 const char *zType;
115875 #ifdef SQLITE_ENABLE_COLUMN_METADATA
115876 const char *zOrigDb = 0;
115877 const char *zOrigTab = 0;
115878 const char *zOrigCol = 0;
115879 zType = columnType(&sNC, p, &zOrigDb, &zOrigTab, &zOrigCol, 0);
115880
115881 /* The vdbe must make its own copy of the column-type and other
115882 ** column specific strings, in case the schema is reset before
this
115883 ** virtual machine is deleted.
115884 */
115885 sqlite3VdbeSetColName(v, i, COLNAME_DATABASE, zOrigDb,
SQLITE_TRANSIENT);
115886 sqlite3VdbeSetColName(v, i, COLNAME_TABLE, zOrigTab,
SQLITE_TRANSIENT);
115887 sqlite3VdbeSetColName(v, i, COLNAME_COLUMN, zOrigCol,
SQLITE_TRANSIENT);
115888 #else
CID 12 301 (#1 of 1): Uninitialized pointer read (UNINIT)
3. uni nit_use_in_call: Using uninitialized value sNC.pNext when calling
columnTypeImpl.
115889 zType = columnType(&sNC, p, 0, 0, 0, 0);
columnType() is a macro expanding to a call to columnTypeImpl().
Anyways, the analysis from here is non-trivial, and I can't convince
myself that sNC.pNext will not be dereferenced.
Thanks for taking the time to look into these.
Some kind of assert() could be helpful there I think. The reason
sNC.pNext will not be accessed is that generateColumnNames() is only
called (a) on a top-level SELECT statement and (b) after all references
have already resolved successfully. Implying that this:
http://www.sqlite.org/src/artifact/672b1af237ad2?ln=1406
is always true.
The obvious fix is to initialize sNC a bit more before the loop at
115872. At least setting sNC.pNext = 0 seems like the right thing to
do.
Another one that I find difficult to analyze is a possible out-of-bounds
read in vdbeSorterCompareInt():
85712 static const u8 aLen[] = {0, 1, 2, 3, 4, 6, 8 };
85713 int i;
85714 res = 0;
85715 for(i=0; i0 && s1<7) || s1==8 || s1==9 );
85701 assert( (s2>0 && s2<7) || s2==8 || s2==9 );
85702
85703 if( s1>7 && s2>7 ){
85704 res = s1 - s2;
85705 }else{
85706 if( s1==s2 ){
At 85715 we know that (s1 <= 7 || s2 <= 7) && s1 == s2, and we also know
that either s1 or s2 can be 8 or 9, so aLen[s1] at 85715 could very well
have s1 > 6, which would read past the bounds of aLen[].
I think ( ( s1<=7 || s2<=7) && s1==s2 ) implies that s1<=7. And we
assume s1!=7 because there is an assert() that says so.
Dan.
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Re: [sqlite] Uninitialized memory reads (not likely false positives)
On 11/15/2016 08:53 PM, Nico Williams wrote:
Another one that I find difficult to analyze is a possible out-of-bounds
read in vdbeSorterCompareInt():
85712 static const u8 aLen[] = {0, 1, 2, 3, 4, 6, 8 };
85713 int i;
85714 res = 0;
85715 for(i=0; i0 && s1<7) || s1==8 || s1==9 );
85701 assert( (s2>0 && s2<7) || s2==8 || s2==9 );
85702
85703 if( s1>7 && s2>7 ){
85704 res = s1 - s2;
85705 }else{
85706 if( s1==s2 ){
At 85715 we know that (s1 <= 7 || s2 <= 7) && s1 == s2, and we also know
that either s1 or s2 can be 8 or 9, so aLen[s1] at 85715 could very well
have s1 > 6, which would read past the bounds of aLen[].
In both of these cases very detailed knowledge of the VDBE being handled
might show that these uninitialized reads do not happen. If so, I don't
have that knowledge.
I'll hold off on other reports for the time being.
Nico
if( s1>7 && s2>7 ){
res = s1 - s2;
}else{
if( s1==s2 ){
// Accesses to aLen as mentioned above
If s1 > 7 && s2 > 7 is false, then at least one of s1 and s2 is not
above 7. If they are equal, then neither s1 nor s2 is above 7.
> and we also know that either s1 or s2 can be 8 or 9,
This is false, unless I am mistaken. See my reasoning above.
The issue is valid, and the message your analyzer (or compiler) wrote is
correct: it is not guaranteed to be < 7, which it should be.
I am unsure whether or not this is actually a bug, but it almost
certainly is a mistake.
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
[sqlite] Uninitialized memory reads (not likely false positives)
I don't normally pay attention to warnings when compiling SQLite3, nor
to Coverity or other static analysis tools' output either, as I'm quite
aware that most of these are false positives and thus unwelcome noise
here.
However, I do sample them occasionally, and though usually such reports
are false positives, here are two that don't quite look like false
positives to me. I get these from building the SQLite3 3.15.1
amalgamation.
Uninitialized pointer dereference:
115861 static void generateColumnTypes(
115862 Parse *pParse, /* Parser context */
115863 SrcList *pTabList, /* List of tables */
115864 ExprList *pEList/* Expressions defining the result set */
115865 ){
115866 #ifndef SQLITE_OMIT_DECLTYPE
115867 Vdbe *v = pParse->pVdbe;
115868 int i;
1. var_decl: Declaring variable sNC without initializer.
115869 NameContext sNC;
115870 sNC.pSrcList = pTabList;
115871 sNC.pParse = pParse;
2. Condition i < pEList->nExpr, taking true branch
115872 for(i=0; inExpr; i++){
115873 Expr *p = pEList->a[i].pExpr;
115874 const char *zType;
115875 #ifdef SQLITE_ENABLE_COLUMN_METADATA
115876 const char *zOrigDb = 0;
115877 const char *zOrigTab = 0;
115878 const char *zOrigCol = 0;
115879 zType = columnType(&sNC, p, &zOrigDb, &zOrigTab, &zOrigCol, 0);
115880
115881 /* The vdbe must make its own copy of the column-type and other
115882 ** column specific strings, in case the schema is reset before
this
115883 ** virtual machine is deleted.
115884 */
115885 sqlite3VdbeSetColName(v, i, COLNAME_DATABASE, zOrigDb,
SQLITE_TRANSIENT);
115886 sqlite3VdbeSetColName(v, i, COLNAME_TABLE, zOrigTab,
SQLITE_TRANSIENT);
115887 sqlite3VdbeSetColName(v, i, COLNAME_COLUMN, zOrigCol,
SQLITE_TRANSIENT);
115888 #else
CID 12 301 (#1 of 1): Uninitialized pointer read (UNINIT)
3. uni nit_use_in_call: Using uninitialized value sNC.pNext when calling
columnTypeImpl.
115889 zType = columnType(&sNC, p, 0, 0, 0, 0);
columnType() is a macro expanding to a call to columnTypeImpl().
Anyways, the analysis from here is non-trivial, and I can't convince
myself that sNC.pNext will not be dereferenced.
The obvious fix is to initialize sNC a bit more before the loop at
115872. At least setting sNC.pNext = 0 seems like the right thing to
do.
Another one that I find difficult to analyze is a possible out-of-bounds
read in vdbeSorterCompareInt():
85712 static const u8 aLen[] = {0, 1, 2, 3, 4, 6, 8 };
85713 int i;
85714 res = 0;
85715 for(i=0; i0 && s1<7) || s1==8 || s1==9 );
85701 assert( (s2>0 && s2<7) || s2==8 || s2==9 );
85702
85703 if( s1>7 && s2>7 ){
85704 res = s1 - s2;
85705 }else{
85706 if( s1==s2 ){
At 85715 we know that (s1 <= 7 || s2 <= 7) && s1 == s2, and we also know
that either s1 or s2 can be 8 or 9, so aLen[s1] at 85715 could very well
have s1 > 6, which would read past the bounds of aLen[].
In both of these cases very detailed knowledge of the VDBE being handled
might show that these uninitialized reads do not happen. If so, I don't
have that knowledge.
I'll hold off on other reports for the time being.
Nico
--
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
