From: drh <> Date: Fri, 20 Oct 2023 17:15:15 +0000 (+0000) Subject: Improvements to the sqlite3ExprDup() logic for faster performance and better X-Git-Tag: version-3.44.0~75 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ab3eb5b770841fe31428d74ceaf6d20ace405f7e;p=thirdparty%2Fsqlite.git Improvements to the sqlite3ExprDup() logic for faster performance and better run-time error detection. This check-in fixes the 5x oversize memory allocation bug from [f371e4c0f8ea73ae] as well as all other known issues that result from handing the ORDER BY clause of an aggregate function off of the pLeft pointer of the Expr object. FossilOrigin-Name: f5c01676fd281e938181b846dd2024d050f597dc6a7a91928beab9d8553dfdb5 --- diff --git a/manifest b/manifest index 9bd609d1f7..a8d211717d 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Add\sthe\sSQLITE_CHANGESETAPPLY_FKNOACTION\sflag\sto\ssqlite3session.h,\sfor\spassing\sto\ssqlite3changeset_apply_v2()\sto\scause\sall\sforeign\skey\sconstraints\sto\sbehave\sas\sif\sthey\swere\sdeclared\sNO\sACTION. -D 2023-10-20T17:06:39.472 +C Improvements\sto\sthe\ssqlite3ExprDup()\slogic\sfor\sfaster\sperformance\sand\sbetter\nrun-time\serror\sdetection.\s\sThis\scheck-in\sfixes\sthe\s5x\soversize\smemory\sallocation\nbug\sfrom\s[f371e4c0f8ea73ae]\sas\swell\sas\sall\sother\sknown\sissues\sthat\sresult\sfrom\nhanding\sthe\sORDER\sBY\sclause\sof\san\saggregate\sfunction\soff\sof\sthe\spLeft\spointer\nof\sthe\sExpr\sobject. +D 2023-10-20T17:15:15.399 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724 @@ -669,7 +669,7 @@ F src/date.c eebc54a00e888d3c56147779e9f361b77d62fd69ff2008c5373946aa1ba1d574 F src/dbpage.c 80e46e1df623ec40486da7a5086cb723b0275a6e2a7b01d9f9b5da0f04ba2782 F src/dbstat.c 3b677254d512fcafd4d0b341bf267b38b235ccfddbef24f9154e19360fa22e43 F src/delete.c cb766727c78e715f9fb7ec8a7d03658ed2a3016343ca687acfcec9083cdca500 -F src/expr.c 1755a8386bd8bb49b5c21cb33682608be33c07ce5df686eb2aa76983bc3a9520 +F src/expr.c 2cf43de9cb6eaed600d689ca0276a8dbb5a56c0fea7e94b2120d9de95d8df309 F src/fault.c 460f3e55994363812d9d60844b2a6de88826e007 F src/fkey.c 360a9b644efc9e05746e0b5b6ccb4760fd039d287ebdf090723b9c97f6d163d9 F src/func.c e8d7b3587a225f4f1116f720b72090511fe9feb936e960bd26a053cea6a17a63 @@ -2134,8 +2134,8 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0 -P 56142a78163b755f16afc05201f623a7a19d9a4b0620a67f7fa20d2a965a288d -R 570cd03c70d6d0573d22a0c9506a275a -U dan -Z 570f4c231d8132d7b5260691ca2afd34 +P fc9f82ea084159eaf3dd1757b96d17d1201b00c4e06455a7dcd8067172b25f28 +R 24ff7529cedac4d1c2b0acdaefc5dd9f +U drh +Z 6942ed424b94726ba0e7696f899b042d # Remove this line to create a well-formed Fossil manifest. diff --git a/manifest.uuid b/manifest.uuid index 8de3ec5dee..e5f3980a50 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -fc9f82ea084159eaf3dd1757b96d17d1201b00c4e06455a7dcd8067172b25f28 \ No newline at end of file +f5c01676fd281e938181b846dd2024d050f597dc6a7a91928beab9d8553dfdb5 \ No newline at end of file diff --git a/src/expr.c b/src/expr.c index aa43c9be7b..c47f7964ae 100644 --- a/src/expr.c +++ b/src/expr.c @@ -1548,45 +1548,61 @@ static int dupedExprSize(const Expr *p){ } /* -** This function is similar to sqlite3ExprDup(), except that if pzBuffer -** is not NULL then *pzBuffer is assumed to point to a buffer large enough -** to store the copy of expression p, the copies of p->u.zToken -** (if applicable), and the copies of the p->pLeft and p->pRight expressions, -** if any. Before returning, *pzBuffer is set to the first byte past the -** portion of the buffer copied into by this function. -*/ -static Expr *exprDup(sqlite3 *db, const Expr *p, int dupFlags, u8 **pzBuffer){ - Expr *pNew; /* Value to return */ - u8 *zAlloc; /* Memory space from which to build Expr object */ - u32 staticFlag; /* EP_Static if space not obtained from malloc */ - +** An EdupBuf is a memory allocation used to stored multiple Expr objects +** together with their Expr.zToken content. This is used to help implement +** compression while doing sqlite3ExprDup(). The top-level Expr does the +** allocation for itself and many of its decendents, then passes an instance +** of the structure down into exprDup() so that they decendents can have +** access to that memory. +*/ +typedef struct EdupBuf EdupBuf; +struct EdupBuf { + u8 *zAlloc; /* Memory space available for storage */ #ifdef SQLITE_DEBUG - /* If zEnd is not NULL, then it is the first byte past the end of the - ** zAlloc buffer allocated by this routine. Used inside assert() - ** to ensure that sufficient space was allocated for zAlloc */ - u8 *zEnd = 0; + u8 *zEnd; /* First byte past the end of memory */ #endif +}; + +/* +** This function is similar to sqlite3ExprDup(), except that if pEdupBuf +** is not NULL then it points to memory that can be used to store a copy +** of the input Expr p together with its p->u.zToken (if any). pEdupBuf +** is updated with the new buffer tail prior to returning. +*/ +static Expr *exprDup( + sqlite3 *db, /* Database connection (for memory allocation) */ + const Expr *p, /* Expr tree to be duplicated */ + int dupFlags, /* EXPRDUP_REDUCE for compression. 0 if not */ + EdupBuf *pEdupBuf /* Preallocated storage space, or NULL */ +){ + Expr *pNew; /* Value to return */ + EdupBuf sEdupBuf; /* Memory space from which to build Expr object */ + u32 staticFlag; /* EP_Static if space not obtained from malloc */ assert( db!=0 ); assert( p ); assert( dupFlags==0 || dupFlags==EXPRDUP_REDUCE ); - assert( pzBuffer==0 || dupFlags==EXPRDUP_REDUCE ); + assert( pEdupBuf==0 || dupFlags==EXPRDUP_REDUCE ); /* Figure out where to write the new Expr structure. */ - if( pzBuffer ){ - zAlloc = *pzBuffer; + if( pEdupBuf ){ + sEdupBuf.zAlloc = pEdupBuf->zAlloc; +#ifdef SQLITE_DEBUG + sEdupBuf.zEnd = pEdupBuf->zEnd; +#endif staticFlag = EP_Static; - assert( zAlloc!=0 ); + assert( sEdupBuf.zAlloc!=0 ); assert( dupFlags==EXPRDUP_REDUCE ); }else{ int nAlloc = dupFlags ? dupedExprSize(p) : dupedExprNodeSize(p, 0); - zAlloc = sqlite3DbMallocRawNN(db, nAlloc*5); + sEdupBuf.zAlloc = sqlite3DbMallocRawNN(db, nAlloc); #ifdef SQLITE_DEBUG - zEnd = zAlloc ? zAlloc+nAlloc : 0; + sEdupBuf.zEnd = sEdupBuf.zAlloc ? sEdupBuf.zAlloc+nAlloc : 0; #endif + staticFlag = 0; } - pNew = (Expr *)zAlloc; + pNew = (Expr *)sEdupBuf.zAlloc; if( pNew ){ /* Set nNewSize to the size allocated for the structure pointed to @@ -1603,16 +1619,18 @@ static Expr *exprDup(sqlite3 *db, const Expr *p, int dupFlags, u8 **pzBuffer){ nToken = 0; } if( dupFlags ){ + assert( (int)(sEdupBuf.zEnd - sEdupBuf.zAlloc) >= nNewSize+nToken ); assert( ExprHasProperty(p, EP_Reduced)==0 ); - memcpy(zAlloc, p, nNewSize); - zAlloc += nNewSize; + memcpy(sEdupBuf.zAlloc, p, nNewSize); + sEdupBuf.zAlloc += nNewSize; }else{ u32 nSize = (u32)exprStructSize(p); - memcpy(zAlloc, p, nSize); + assert( (int)(sEdupBuf.zEnd - sEdupBuf.zAlloc) >= EXPR_FULLSIZE+nToken ); + memcpy(sEdupBuf.zAlloc, p, nSize); if( nSizeu.zToken string, if any. */ if( nToken ){ - char *zToken = pNew->u.zToken = (char*)zAlloc; + char *zToken = pNew->u.zToken = (char*)sEdupBuf.zAlloc; memcpy(zToken, p->u.zToken, nToken); - zAlloc += nToken; + sEdupBuf.zAlloc += nToken; } - if( 0==((p->flags|pNew->flags) & (EP_TokenOnly|EP_Leaf)) ){ + if( ((p->flags|pNew->flags)&(EP_TokenOnly|EP_Leaf))==0 ){ + /* Fill in the pNew->x.pSelect or pNew->x.pList member. */ if( ExprUseXSelect(p) ){ pNew->x.pSelect = sqlite3SelectDup(db, p->x.pSelect, dupFlags); @@ -1639,31 +1658,33 @@ static Expr *exprDup(sqlite3 *db, const Expr *p, int dupFlags, u8 **pzBuffer){ pNew->x.pList = sqlite3ExprListDup(db, p->x.pList, p->op!=TK_ORDER ? dupFlags : 0); } - } - /* Fill in pNew->pLeft and pNew->pRight. */ - if( ExprHasProperty(pNew, EP_Reduced|EP_TokenOnly|EP_WinFunc) ){ - if( !ExprHasProperty(pNew, EP_TokenOnly|EP_Leaf) ){ - pNew->pLeft = p->pLeft ? - exprDup(db, p->pLeft, EXPRDUP_REDUCE, &zAlloc) : 0; - pNew->pRight = p->pRight ? - exprDup(db, p->pRight, EXPRDUP_REDUCE, &zAlloc) : 0; - } #ifndef SQLITE_OMIT_WINDOWFUNC if( ExprHasProperty(p, EP_WinFunc) ){ pNew->y.pWin = sqlite3WindowDup(db, pNew, p->y.pWin); assert( ExprHasProperty(pNew, EP_WinFunc) ); } #endif /* SQLITE_OMIT_WINDOWFUNC */ - if( pzBuffer ){ - *pzBuffer = zAlloc; - } - }else{ - if( !ExprHasProperty(p, EP_TokenOnly|EP_Leaf) ){ - if( pNew->op==TK_SELECT_COLUMN ){ + + /* Fill in pNew->pLeft and pNew->pRight. */ + if( dupFlags ){ + if( p->op==TK_SELECT_COLUMN ){ + pNew->pLeft = p->pLeft; + assert( p->pRight==0 + || p->pRight==p->pLeft + || ExprHasProperty(p->pLeft, EP_Subquery) ); + }else{ + pNew->pLeft = p->pLeft ? + exprDup(db, p->pLeft, EXPRDUP_REDUCE, &sEdupBuf) : 0; + } + pNew->pRight = p->pRight ? + exprDup(db, p->pRight, EXPRDUP_REDUCE, &sEdupBuf) : 0; + }else{ + if( p->op==TK_SELECT_COLUMN ){ pNew->pLeft = p->pLeft; - assert( p->pRight==0 || p->pRight==p->pLeft - || ExprHasProperty(p->pLeft, EP_Subquery) ); + assert( p->pRight==0 + || p->pRight==p->pLeft + || ExprHasProperty(p->pLeft, EP_Subquery) ); }else{ pNew->pLeft = sqlite3ExprDup(db, p->pLeft, 0); } @@ -1671,7 +1692,8 @@ static Expr *exprDup(sqlite3 *db, const Expr *p, int dupFlags, u8 **pzBuffer){ } } } - assert( zEnd==0 || zAlloc<=zEnd ); + if( pEdupBuf ) memcpy(pEdupBuf, &sEdupBuf, sizeof(sEdupBuf)); + assert( sEdupBuf.zAlloc <= sEdupBuf.zEnd ); return pNew; }