Fix assorted correctness bugs found via a code-review sweep

dev/ese/published/inc/stat.hxx

@@ -775,7 +775,7 @@ public:
         {
             return ERR::wrnOutOfSamples;
         }
-        else if ( m_iValueLastQuery > 1 &&
+        else if ( m_iValueLastQuery > 0 &&
                     qwValue < m_pValues[m_iValueLastQuery-1] )
         {
             return ERR::errInvalidParameter;
@@ -1276,7 +1276,7 @@ class CSegmentedHistogram : public CStats {
             {
                 return ERR::wrnOutOfSamples;
             }
-            else if ( m_iValueLastQuery > 1 &&
+            else if ( m_iValueLastQuery > 0 &&
                         qwValue < m_pValues[m_iValueLastQuery-1] )
             {
                 return ERR::errInvalidParameter;

dev/ese/src/_xpress9/Xpress9DecLz77.c

@@ -1045,8 +1045,8 @@ Xpress9DecoderFetchDecompressedData (
                     "Buffer too small to DirectDecode, uBytesNeeded=%Iu, uBytesToCopy=%Iu, uDecodePosition=%Iu, uBufferDataSize=%Iu",
                     uBytesNeeded,
                     uBytesToCopy,
-                    pDecoder->m_DecodeData.m_uDecodePosition
-                    pDecoder->m_BufferData.m_uBufferDataSize,
+                    pDecoder->m_DecodeData.m_uDecodePosition,
+                    pDecoder->m_BufferData.m_uBufferDataSize
                 );
                 goto Failure;
             }

dev/ese/src/ese/_log/logread.cxx

@@ -1071,8 +1071,16 @@ LOG_VERIFY_STATE::ErrVerifyHeader( INST * pinst, IFileAPI * pfapi, __deref_in_bc
     {
         m_state = LogVerifyLogSegments;
         m_cbSeg = plgfilehdr->lgfilehdr.le_cbSec;
-        *ppb += plgfilehdr->lgfilehdr.le_csecHeader * m_cbSeg;
-        *pcb -= plgfilehdr->lgfilehdr.le_csecHeader * m_cbSeg;
+        //  le_cbSec / le_csecHeader are persisted (untrusted) header fields; a zero sector size or a
+        //  header span larger than the buffer would underflow *pcb and walk past the buffer in
+        //  ErrVerifyLogSegments below.
+        const QWORD cbHeaderSpan = (QWORD)plgfilehdr->lgfilehdr.le_csecHeader * m_cbSeg;
+        if ( m_cbSeg == 0 || cbHeaderSpan > *pcb )
+        {
+            Error( ErrERRCheck( JET_errLogReadVerifyFailure ) );
+        }
+        *ppb += (DWORD)cbHeaderSpan;
+        *pcb -= (DWORD)cbHeaderSpan;
         m_iSeg = plgfilehdr->lgfilehdr.le_csecHeader;
     }
 
@@ -2337,8 +2345,11 @@ ERR LOG_READ_BUFFER::ErrLGIGetRecordAtPbNext( BYTE **ppbLR, BOOL fPreread, BOOL
                     Error( ErrERRCheck( JET_errLogFileCorrupt ) );
                 }
                 else if ( CbLGFixedSizeOfRec( (LR *)pbCurrent ) > (DWORD)( pbSegment + m_pLogStream->CbSec() - pbCurrent ) ||
-                        CbLGSizeOfRec( (LR *)pbCurrent ) > (DWORD)( pbSegment + m_pLogStream->CbSec() - pbCurrent ) )
+                        CbLGSizeOfRec( (LR *)pbCurrent ) > (DWORD)( pbSegment + m_pLogStream->CbSec() - pbCurrent ) ||
+                        0 == CbLGSizeOfRec( (LR *)pbCurrent ) )
                 {
+                    //  a zero-size record (e.g. an obsolete/unknown in-range lrtyp) would not advance
+                    //  pbCurrent below, causing the caller to loop forever -- treat it as corruption.
                     AssertSz( FNegTest( fCorruptingLogFiles ), "Corrupt size of LR (%d,%d) past rest of segment (%d).",
                                     CbLGFixedSizeOfRec( (LR *)pbCurrent ),
                                     CbLGSizeOfRec( (LR *)pbCurrent ),

dev/ese/src/ese/_log/logredo.cxx

@@ -237,6 +237,14 @@ LOCAL ERR ErrReplacePageImageHeaderTrailer(
                                 const DBTIME dbtimeBefore )
 {
 
+    //  cbHeader/cbTrailer come from a persisted (possibly corrupt) log record; reject a record
+    //  that would overrun the g_cbPage rebuild buffer (and underflow the memset in
+    //  RebuildPageImageHeaderTrailer) before touching the buffer.
+    if ( cbHeader < 0 || cbTrailer < 0 || cbHeader > g_cbPage || cbTrailer > g_cbPage || cbHeader + cbTrailer > g_cbPage )
+    {
+        return ErrERRCheck( JET_errLogFileCorrupt );
+    }
+
     VOID * pvBuffer;
     BFAlloc( bfasTemporary, &pvBuffer );
 
@@ -8076,6 +8084,13 @@ ERR LOG::ErrLGRIRedoInitializeSplit( PIB * const ppib, const LRSPLIT_ * const pl
     {
         const INT   cbKeyParent = plrsplit->le_cbKeyParent;
 
+        //  cbKeyParent is from a persisted (possibly corrupt) log record; bound it to the
+        //  RESBOOKMARK buffer capacity before copying to avoid a heap overflow.
+        if ( cbKeyParent > cbBookmarkAlloc )
+        {
+            Error( ErrERRCheck( JET_errLogCorrupted ) );
+        }
+
         psplit->kdfParent.key.suffix.SetPv( RESBOOKMARK.PvRESAlloc() );
         Alloc( psplit->kdfParent.key.suffix.Pv() );
 
@@ -8093,6 +8108,13 @@ ERR LOG::ErrLGRIRedoInitializeSplit( PIB * const ppib, const LRSPLIT_ * const pl
     {
         const INT   cbPrefix = plrsplit->le_cbPrefixSplitOld;
 
+        //  cbPrefix is from a persisted (possibly corrupt) log record; bound it to the
+        //  RESBOOKMARK buffer capacity before copying to avoid a heap overflow.
+        if ( cbPrefix > cbBookmarkAlloc )
+        {
+            Error( ErrERRCheck( JET_errLogCorrupted ) );
+        }
+
         psplit->prefixSplitOld.SetPv( RESBOOKMARK.PvRESAlloc() );
         Alloc( psplit->prefixSplitOld.Pv() );
 
@@ -8107,6 +8129,13 @@ ERR LOG::ErrLGRIRedoInitializeSplit( PIB * const ppib, const LRSPLIT_ * const pl
     {
         const INT   cbPrefix = plrsplit->le_cbPrefixSplitNew;
 
+        //  cbPrefix is from a persisted (possibly corrupt) log record; bound it to the
+        //  RESBOOKMARK buffer capacity before copying to avoid a heap overflow.
+        if ( cbPrefix > cbBookmarkAlloc )
+        {
+            Error( ErrERRCheck( JET_errLogCorrupted ) );
+        }
+
         psplit->prefixSplitNew.SetPv( RESBOOKMARK.PvRESAlloc() );
         Alloc( psplit->prefixSplitNew.Pv() );
 
@@ -8461,6 +8490,13 @@ ERR LOG::ErrLGRIRedoInitializeMerge( PIB            *ppib,
     {
         const INT   cbKeyParentSep = plrmerge->le_cbKeyParentSep;
 
+        //  cbKeyParentSep is from a persisted (possibly corrupt) log record; bound it to the
+        //  RESBOOKMARK buffer capacity before copying to avoid a heap overflow.
+        if ( cbKeyParentSep > cbBookmarkAlloc )
+        {
+            Error( ErrERRCheck( JET_errLogCorrupted ) );
+        }
+
         pmerge->kdfParentSep.key.suffix.SetPv( RESBOOKMARK.PvRESAlloc() );
         Alloc( pmerge->kdfParentSep.key.suffix.Pv() );
 
@@ -10590,8 +10626,12 @@ ERR LOG::ErrLGIRedoRootMoveStructures( PIB* const ppib, const DBTIME dbtime, ROO
 
                     // Copy new data.
                     const USHORT cbData = plrseh->CbData();
-                    Assert( cbData == sizeof( prmc->sphNew ) );
-                    Assert( cbData == prmc->dataSphNew.Cb() );
+                    //  cbData is from a persisted (possibly corrupt) log record; the destination
+                    //  is a fixed-size SPACE_HEADER, so reject a mismatched length.
+                    if ( cbData != sizeof( prmc->sphNew ) )
+                    {
+                        Error( ErrERRCheck( JET_errLogCorrupted ) );
+                    }
                     UtilMemCpy( prmc->dataSphNew.Pv(), plrseh->rgbData, cbData );
                 }
             }

dev/ese/src/ese/_log/logwrite.cxx

@@ -431,8 +431,8 @@ ERR LOG_BUFFER::InitCommit( LONG cBytes )
     {
         Error( ErrERRCheck( JET_errOutOfMemory ) );
     }
-    Assert( _pbLGCommitStart = _pbLGBufMin );
-    Assert( _pbLGCommitEnd = _pbLGBufMin + roundup( cBytes, OSMemoryPageCommitGranularity() ) - 1 );
+    Assert( _pbLGCommitStart == _pbLGBufMin );
+    Assert( _pbLGCommitEnd == _pbLGBufMin + roundup( cBytes, OSMemoryPageCommitGranularity() ) - 1 );
 
 HandleError:
     return err;

dev/ese/src/ese/_log/rstmap.cxx

@@ -939,7 +939,7 @@ BOOL LOG::FRstmapCheckDuplicateSignature(  )
 
         for ( irstmapSearch = irstmap + 1; irstmapSearch < m_irstmapMac; irstmapSearch++ )
         {
-            if ( !m_rgrstmap[irstmap].wszNewDatabaseName )
+            if ( !m_rgrstmap[irstmapSearch].wszNewDatabaseName )
                 continue;
 
             if ( 0 == memcmp(   &m_rgrstmap[irstmap].signDatabase,

dev/ese/src/ese/backup.cxx

@@ -1198,7 +1198,10 @@ ERR BACKUP_CONTEXT::ErrBKIPrepareDirectory(
             {
                 Call( ErrERRCheck( JET_errInvalidPath ) );
             }
-            OSStrCbCopyW( wszBackupPath, cbBackupPath, wszAtomicOld );
+            //  append the "old" subdirectory onto the (already normalized) base backup path rather
+            //  than overwriting it -- the length check above sums both lengths, and the full-backup
+            //  branch below likewise appends its subdirectory.
+            OSStrCbAppendW( wszBackupPath, cbBackupPath, wszAtomicOld );
             CallS( m_pinst->m_pfsapi->ErrPathFolderNorm( wszBackupPath, cbBackupPath ) );
         }
         else

dev/ese/src/ese/cat.cxx

@@ -6493,7 +6493,7 @@ LOCAL ERR ErrCATIBuildFIELDArray(
         if ( fFixedColumn )
         {
             Assert( FidOfColumnid( columnid ) >= ptdb->FidFixedFirst() );
-            Assert( FidOfColumnid( columnid ) <= ptdb->FidTaggedLast() );
+            Assert( FidOfColumnid( columnid ) <= ptdb->FidFixedLast() );
         }
 
         if ( !FNegTest( fInvalidAPIUsage ) && FFIELDFinalize( field.ffield ) && !s_fLimitFinalizeFfieldNyi )
@@ -15402,9 +15402,17 @@ ERR ErrCATGetNextRootObject(
     {
         // Retrieve the object name.
         Call( ErrRECIRetrieveVarColumn( pfcbNil, pfucbCatalog->u.pfcb->Ptdb(), fidMSO_Name, pfucbCatalog->kdfCurr.data, &dataField ) );
-        Assert( ( dataField.Cb() / sizeof( szObjectNameT[ 0 ] ) ) < _countof( szObjectNameT ) );
-        UtilMemCpy( szObjectNameT, dataField.Pv(), dataField.Cb() );
-        szObjectNameT[ dataField.Cb() / sizeof( szObjectNameT[0] ) ] = '\0';
+        const INT cbObjectName = dataField.Cb();
+        if ( cbObjectName < 0 || cbObjectName > JET_cbNameMost )
+        {
+            //  the persisted name length is untrusted; reject a corrupt catalog rather than
+            //  overrun the fixed-size stack buffer (matches the other fidMSO_Name readers).
+            AssertSz( fFalse, "Invalid fidMSO_Name column in catalog." );
+            OSUHAEmitFailureTag( PinstFromPfucb( pfucbCatalog ), HaDbFailureTagCorruption, L"7d2c1f8a-6b34-4e05-9c1a-3f8e2d6b40a9" );
+            Error( ErrERRCheck( JET_errCatalogCorrupted ) );
+        }
+        UtilMemCpy( szObjectNameT, dataField.Pv(), cbObjectName );
+        szObjectNameT[ cbObjectName ] = '\0';
     }
 
 HandleError:

dev/ese/src/ese/comp.cxx

@@ -1240,6 +1240,13 @@ LOCAL ERR ErrCMPCopyTable(
         const ULONG cpgUsed = rgcpgExtent[0] - rgcpgExtent[1];
         Assert( cpgUsed > 0 );
 
+        //  a (corrupt) space tree reporting AvailExt == OwnExt would make cpgUsed 0 and divide by
+        //  zero below; treat it as corruption so the meter progression is suppressed instead.
+        if ( 0 == cpgUsed )
+        {
+            fCorruption = fTrue;
+        }
+
         pstatus->cbRawData = 0;
         pstatus->cbRawDataLV = 0;
         pstatus->cLeafPagesTraversed = 0;

dev/ese/src/ese/compression.cxx

@@ -2096,7 +2096,7 @@ ERR CDataCompressor::ErrCompressLz4_(
     Assert( cbCompressedActual <= cbDataCompressedMax - cbReserved );
 
     PERFOpt( pstats->AddUncompressedBytes( data.Cb() ) );
-    PERFOpt( pstats->AddCompressedBytes( *pcbDataCompressedActual ) );
+    PERFOpt( pstats->AddCompressedBytes( cbCompressedActual + cbReserved ) );
     PERFOpt( pstats->IncCompressionCalls() );
     PERFOpt( pstats->AddCompressionDhrts( HrtHRTCount() - hrtStart ) );
 

dev/ese/src/ese/cpage.cxx

@@ -6992,7 +6992,9 @@ ERR CPAGE::DumpHeader( CPRINTF * pcprintf, DWORD_PTR dwOffset ) const
                 // calculate the expected tag size
                 USHORT usExpectedTagSize = 1; //for the flag byte
                 BYTE fFlagT = 0x1;
-                for ( INT i = 0; i < noderfMax; ++i, fFlagT <<= 1 )
+                //  g_rgcbExternalHeaderSize has noderfMax entries (0..noderfMax-1); i indexes [i+1],
+                //  so stop at noderfMax-1 to avoid reading one past the end on a corrupt flag byte.
+                for ( INT i = 0; i < noderfMax - 1; ++i, fFlagT <<= 1 )
                 {
                     if ( fFlagT & fNodeFlag )
                     {

dev/ese/src/ese/dataserializer.cxx

@@ -903,8 +903,12 @@ ERR MemoryDataStore::ErrStoreDataToColumn( const char * const szColumn, const vo
     else
     {
         BYTE * pb = new BYTE[cb];
+        if ( pb == NULL )
+        {
+            return ErrERRCheck( JET_errOutOfMemory );
+        }
         memcpy( pb, pv, cb );
-        m_rgpbData[i] = unique_ptr<BYTE>( pb );
+        m_rgpbData[i] = unique_ptr<BYTE[]>( pb );
         m_rgcbData[i] = cb;
         return JET_errSuccess;
     }

dev/ese/src/ese/dbscan.cxx

@@ -1733,6 +1733,8 @@ CDBMScanFollower::CDBMScanFollower()
     m_pstate = NULL;
     m_pscanobsFileCheck = NULL;
     m_pscanobsLgriEvents = NULL;
+    m_pscanobsPerfmon = NULL;
+    m_cFollowerSkipsPrePass = 0;
 }
 
 CDBMScanFollower::~CDBMScanFollower()
@@ -3981,7 +3983,7 @@ ERR DBMScanObserverCleanup::ErrCleanupPrimaryPage_( CSR * const pcsr, DBMObjectC
 
                 // Extract the bookmark
                 // Call ErrBTDelete
-                unique_ptr<BYTE> pbBookmark( new BYTE[kdf.key.Cb()] );
+                unique_ptr<BYTE[]> pbBookmark( new BYTE[kdf.key.Cb()] );
                 Alloc( pbBookmark.get() );
                 kdf.key.CopyIntoBuffer( pbBookmark.get(), kdf.key.Cb() );
 

dev/ese/src/ese/dbutil.cxx

@@ -1137,7 +1137,7 @@ LOCAL_BROKEN ERR ErrDBUTLMungeDatabase(
         {
             return ErrERRCheck( JET_errInvalidParameter );
         }
-        const PGNO pgnoNext = strtoul( rgszCommand[2], NULL, 0 );
+        const PGNO pgnoNext = strtoul( rgszCommand[2], &pchEnd, 0 );
         if( 0 != *pchEnd )
         {
             return ErrERRCheck( JET_errInvalidParameter );
@@ -1155,7 +1155,7 @@ LOCAL_BROKEN ERR ErrDBUTLMungeDatabase(
         {
             return ErrERRCheck( JET_errInvalidParameter );
         }
-        const PGNO pgnoPrev = strtoul( rgszCommand[2], NULL, 0 );
+        const PGNO pgnoPrev = strtoul( rgszCommand[2], &pchEnd, 0 );
         if( 0 != *pchEnd )
         {
             return ErrERRCheck( JET_errInvalidParameter );
@@ -1173,7 +1173,7 @@ LOCAL_BROKEN ERR ErrDBUTLMungeDatabase(
         {
             return ErrERRCheck( JET_errInvalidParameter );
         }
-        const ULONG fFlags = strtoul( rgszCommand[2], NULL, 0 );
+        const ULONG fFlags = strtoul( rgszCommand[2], &pchEnd, 0 );
         if( 0 != *pchEnd )
         {
             return ErrERRCheck( JET_errInvalidParameter );

dev/ese/src/ese/dir.cxx

@@ -2588,7 +2588,14 @@ LOCAL ERR ErrDIRIIRefresh( FUCB * const pfucb )
         bm.key.prefix.Nullify();
         bm.key.suffix = pfucbIdx->kdfCurr.data;
 
-        Call( ErrDIRGotoBookmark( pfucb, bm ) );
+        err = ErrDIRGotoBookmark( pfucb, bm );
+        if ( err < 0 )
+        {
+            //  pfucbIdx is a distinct secondary-index cursor left read-latched by ErrBTDown above;
+            //  release its latch before erroring out so we don't leak the page latch.
+            CallS( ErrBTRelease( pfucbIdx ) );
+            goto HandleError;
+        }
     }
 
     Call( ErrBTRelease( pfucbIdx ) );

dev/ese/src/ese/fcreate.cxx

@@ -7675,7 +7675,7 @@ LOCAL ERR VTAPI ErrFILEIBatchCreateIndex(
         }
 
         // Force a write to the output structure (but don't actually modify the value).
-        OnDebug( AtomicCompareExchange( &pidxcreate->err, pidxcreate->err, pidxcreate->err ) );
+        OnDebug( AtomicCompareExchange( &pidxcreateT->err, pidxcreateT->err, pidxcreateT->err ) );
 
         if ( fLazyCommit && !( pidxcreateT->grbit & JET_bitIndexLazyFlush ) )
         {
@@ -7688,7 +7688,7 @@ LOCAL ERR VTAPI ErrFILEIBatchCreateIndex(
         pfcb->GetAPISpaceHints( &jsphIndex );
         if ( pidxcreateT->pSpacehints )
         {
-            jsphIndex = *(pidxcreate->pSpacehints);
+            jsphIndex = *(pidxcreateT->pSpacehints);
         }
         if ( ( (ULONG)g_cbPage * cpgInitialTreeDefault ) == jsphIndex.cbInitial )
         {
@@ -8349,7 +8349,7 @@ ERR VTAPI ErrIsamCreateIndex(
         {
             Assert( sizeof(JET_INDEXCREATE3_A) == pindexcreateT->cbStruct );
 
-            if ( pindexcreate->grbit & JET_bitIndexDeferredPopulateProcess )
+            if ( pindexcreateT->grbit & JET_bitIndexDeferredPopulateProcess )
             {
                 // Trying to continue processing a deferred-populate index.
 
@@ -8368,12 +8368,12 @@ ERR VTAPI ErrIsamCreateIndex(
                     Error( ErrERRCheck( JET_errIllegalOperation ) );
                 }
 
-                Call( ErrFILEIProcessDeferredPopulateIndex( ppib, pfucbTable, pindexcreate ) );
+                Call( ErrFILEIProcessDeferredPopulateIndex( ppib, pfucbTable, pindexcreateT ) );
             }
             else
             {
                 // Trying to create a single index, deferred-populate or not.
-                if ( pindexcreate->grbit & JET_bitIndexDeferredPopulateCreate )
+                if ( pindexcreateT->grbit & JET_bitIndexDeferredPopulateCreate )
                 {
                     // Trying to create a single deferred-populate index.
                     if ( !fAllowDeferred )
@@ -8391,7 +8391,7 @@ ERR VTAPI ErrIsamCreateIndex(
                     Assert ( pfmp->PkvpsMSysDeferredPopulateKeys() );
                 }
 
-                Call( ErrFILEICreateIndex( ppib, pfucbTable, pindexcreate ) );
+                Call( ErrFILEICreateIndex( ppib, pfucbTable, pindexcreateT ) );
             }
             fLazyCommit = fLazyCommit && ( pindexcreateT->grbit & JET_bitIndexLazyFlush );
         }

dev/ese/src/ese/fileopen.cxx

@@ -2700,6 +2700,13 @@ ERR ErrIDBSetIdxSeg(
         return JET_errSuccess;
     }
 
+    //  cidxseg is derived from a persisted catalog column length; a corrupt value larger than
+    //  the fixed rgidxseg[] stack array would overflow it in the conversion loop below.
+    if ( cidxseg > _countof( rgidxseg ) )
+    {
+        return ErrERRCheck( JET_errCatalogCorrupted );
+    }
+
     //  If it is on little endian machine, we still copy it into
     //  the stack array which is aligned.
     //  If it is on big endian machine, we always need to convert first.
@@ -2800,6 +2807,13 @@ ERR ErrIDBSetIdxSegFromOldFormat(
     TCIB        tcibTemplateTable           = { FID( ptdb->FidFixedFirst()-1 ),
                                                 FID( ptdb->FidVarFirst()-1 ),
                                                 FID( ptdb->FidTaggedFirst()-1 ) };
+
+    //  cidxseg is derived from a persisted catalog column length; a corrupt value larger than
+    //  the fixed rgidxseg[] stack array would overflow it in SetIdxSegFromOldFormat below.
+    if ( cidxseg > _countof( rgidxseg ) )
+    {
+        return ErrERRCheck( JET_errCatalogCorrupted );
+    }
 #ifdef DEBUG
     if ( ptdb->FDerivedTable() )
     {