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 9bd609d1f7f9bc082a87778e6c32d7c11677e123..a8d211717d8aca7446f1046ff05d412adc9d9797 100644 (file)
--- 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 8de3ec5deee7efd95297e8a8836a29c6c2724ea7..e5f3980a50b0cce95c31261106eebe1a79b51975 100644 (file)
--- 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 aa43c9be7bcc5ed88c70ccbf18cf80e34f28dae7..c47f7964ae3c090a4fb3fd4fe5cbbfb59a8aa310 100644 (file)
--- 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( nSize<EXPR_FULLSIZE ){
-        memset(&zAlloc[nSize], 0, EXPR_FULLSIZE-nSize);
+        memset(&sEdupBuf.zAlloc[nSize], 0, EXPR_FULLSIZE-nSize);
       }
-      zAlloc += EXPR_FULLSIZE;
+      sEdupBuf.zAlloc += EXPR_FULLSIZE;
     }
 
     /* Set the EP_Reduced, EP_TokenOnly, and EP_Static flags appropriately. */
@@ -1626,12 +1644,13 @@ static Expr *exprDup(sqlite3 *db, const Expr *p, int dupFlags, u8 **pzBuffer){
 
     /* Copy the p->u.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;
 }