SQLite Forum

several potential NULL pointer dereferences
Login

several potential NULL pointer dereferences

(1.1) By YJRUC (YuanjunGongRUC) on 2020-11-08 11:18:06 edited from 1.0 [source]

Hi Team, 

There are potential NULL pointer dereferences, checks should be implemented to the return value of function sqlite3ExprSkipCollateAndLikely. A similar problem has been fixed in [check-in:5aeb5a2d].

In function resolveCompoundOrderBy in /src/resolve.c,           

    Expr *pE, *pDup;
    if( pItem->done ) continue;
    pE = sqlite3ExprSkipCollateAndLikely(pItem->pExpr);
    if( sqlite3ExprIsInteger(pE, &iCol) ){
      if( iCol<=0 || iCol>pEList->nExpr ){
        resolveOutOfRangeError(pParse, "ORDER", i+1, pEList->nExpr);
        return 1;
      }
    }else{
      iCol = resolveAsName(pParse, pEList, pE);


In function resolveCompoundOrderBy in /src/resolve.c,

    Expr *pE2 = sqlite3ExprSkipCollateAndLikely(pE);
    if( zType[0]!='G' ){
    iCol = resolveAsName(pParse, pSelect->pEList, pE2);


In function findIndexCol in /src/where.c,

    Expr *p = sqlite3ExprSkipCollateAndLikely(pList->a[i].pExpr);
    if( p->op==TK_COLUMN
     && p->iColumn==pIdx->aiColumn[iCol]
     && p->iTable==iBase


In function isDistinctRedundant in /src/where.c,

    for(i=0; i<pDistinct->nExpr; i++){
        Expr *p = sqlite3ExprSkipCollateAndLikely(pDistinct->a[i].pExpr);
        if( p->op==TK_COLUMN && p->iTable==iBase && p->iColumn<0 ) return 1;
    }


In function indexMightHelpWithOrderBy in /src/where.c,
    
    Expr *pExpr = sqlite3ExprSkipCollateAndLikely(pOB->a[ii].pExpr);
    if( pExpr->op==TK_COLUMN && pExpr->iTable==iCursor ){
      if( pExpr->iColumn<0 ) return 1;
      for(jj=0; jj<pIndex->nKeyCol; jj++){
        if( pExpr->iColumn==pIndex->aiColumn[jj] ) return 1;
      }


In function exprToRegister in /src/expr.c,

    Expr *p = sqlite3ExprSkipCollateAndLikely(pExpr);
    p->op2 = p->op;
    p->op = TK_REGISTER;
    p->iTable = iReg;
    ExprClearProperty(p, EP_Skip);


In function sqlite3ExprCodeTemp in /src/expr.c,

    pExpr = sqlite3ExprSkipCollateAndLikely(pExpr);
    if( ConstFactorOk(pParse)
     && pExpr->op!=TK_REGISTER
     && sqlite3ExprIsConstantNotJoin(pExpr)
    ){

(2) By Larry Brasfield (LarryBrasfield) on 2020-11-08 14:51:45 in reply to 1.1 [link] [source]

I think you are missing something important in your "potential" problem diagnosis. A hint as to what that something is appears in the comment header for the function you suggest might return a NULL reference: /* ** Skip over any TK_COLLATE operators and/or any unlikely() ** or likelihood() or likely() functions at the root of an ** expression. */ What this means is that the function is not allocating anything, and cannot return NULL due to its own (or its callees') allocation failure. It merely walks some part of an expression tree to ignore some part. Or, if passed NULL, it returns NULL. As long as the part to be ignored did not incorporate a NULL expression (sub)tree, the sqlite3ExprSkipCollateAndLikely(pExpr) call is not going to return NULL. Whether that can happen or not is a question of parser correctness and correctness of any parse tree manipulations that are done.

On the possibility that sqlite3ExprSkipCollateAndLikely(pExpr) is called with pExpr == NULL, I think you need to show that such an actual argument has some real prospect of occurring before issuing "should" recommendations for a bunch of additional conditionals in oft-executed code. My perusal of a subset of those calls has shown that, at the point of sqlite3ExprSkipCollateAndLikely() calls, the argument is well past the point in the code where it might be NULL.

I was curious about your assertion, "[a] similar problem has been fixed". (The referenced checkin is here.) There, a subtree is passed to sqlite3ExprSkipCollateAndLikely(). I do not see that said change was a fix for a "similar problem." Whether an expression tree or subtree can contain NULL children is very dependent upon rules (or invariants) being maintained in specific parts of the code or for specific subtree types. Before I would go along with your "similar" assertion, I would need to see at least the same (or similar) subtree types being manipulated, and I would also want to know whether possible OOM events have already been handled before that code can be reached.

From what I have seen of the SQLite codebase, I believe that even if you have hit a problem with your scattershot diagnosis, the solution would be narrowly targeted adherence to existing invariants rather than a bunch of likely useless NULL checks.

(3) By Richard Hipp (drh) on 2020-11-08 20:51:57 in reply to 1.1 [link] [source]

There are potential NULL pointer dereferences

No, there is not. Though, the situation is complex and the proof that sqlite3ExprSkipCollateAndLikely() can never return NULL in the contexts you site is difficult and probably beyond the capability of your static analyzer. For that reason, I did put in checks to verify that the result is not NULL. See check-in 76d2eb86e109fc3c.

Notice, however, that all of these checks must be put inside of NEVER() or ALWAYS() (depending on the sense of the check) since the conditional is always false or true, respectively. Without the use of the NEVER() and ALWAYS() macros, we would be left with unreachable branches and our branch coverage testing would fail.

To summarize, then: This is a false-positive in your static analyzer, not an actual bug. Nevertheless, it does provide an opportunity to make the code simpler to understand and prove correct, and so appropriate adjustments where made to that end.