L:401 ... L:366 ...

For future reference: when referring to the source code, always provide the hash of the checkin you're referring to, as without that the line numbers may well not match what we're looking at.

va_end is not called before ...

That one is now resolved. The others will require closer evaluation. Thank you for providing such detailed analysis!

PS:

I won't post the code here, since it would just complicate your work (in case it is an actual problem) since you would have to come up with a different fix to not use code you are not completely sure is public domain.

It's okay to post patches here. Though we cannot take them verbatim, they are often treated as a proof-of-concept and reimplemented.

Since I am a new SQLite maintainer for Fedora, I was thinking whether it would be possible to submit the 'affidavit' you mention at https://sqlite.org/copyright.html so in future cases I would be able to post PRs. If you have any document prepared it would have to go through Red Hat Legal team first, but I couldn't find any document like this on SQLite website, so I thought it would be easier to ask first here whether you even allow this.

Please contact the project lead directly about that: drh at this domain.

Oh, right, I didn't realize that, my apologies. Just to clarify now, all line numbers are referencing code from published sqlite-src-3460000.zip. So published code version 3.46.0 (so as far as I know 96c92aba00c8375bc32fafcdf12429c58bd8aabfcadab6683e35bbb9cdebf19e should be the hash of checkin)

Alright, thank you very much for the clarification. I will contact the project lead at the provided address.

Here is the fix that should resolve the issue 3. It does pass all tests when checked both via the reproducer I mentioned and without the reproducer. There probably is a better way, but from my testing it does work. At least I think it could work as a proof of concept as you said.

From: Ales Nezbeda <anezbeda@redhat.com>
Date: Wed, 7 Aug 2024 09:56:46 +0200
Subject: [PATCH] Fix possible segmentation fault and double free in
 sqlite3Fts5ParseNode

---
 ext/fts5/fts5_expr.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/ext/fts5/fts5_expr.c b/ext/fts5/fts5_expr.c
index 05c1b59..b6c7428 100644
--- a/ext/fts5/fts5_expr.c
+++ b/ext/fts5/fts5_expr.c
@@ -2258,19 +2258,19 @@ static void fts5ExprAssignXNext(Fts5ExprNode *pNode){
   }
 }
 
-static void fts5ExprAddChildren(Fts5ExprNode *p, Fts5ExprNode *pSub){
+static int fts5ExprAddChildren(Fts5ExprNode *p, Fts5ExprNode *pSub){
   int ii = p->nChild;
   if( p->eType!=FTS5_NOT && pSub->eType==p->eType ){
     int nByte = sizeof(Fts5ExprNode*) * pSub->nChild;
     memcpy(&p->apChild[p->nChild], pSub->apChild, nByte);
     p->nChild += pSub->nChild;
-    sqlite3_free(pSub);
   }else{
     p->apChild[p->nChild++] = pSub;
   }
   for( ; ii<p->nChild; ii++){
     p->iHeight = MAX(p->iHeight, p->apChild[ii]->iHeight + 1);
   }
+  return p->eType!=FTS5_NOT && pSub->eType==p->eType;
 }
 
 /*
@@ -2407,8 +2407,8 @@ Fts5ExprNode *sqlite3Fts5ParseNode(
             }
           }
         }else{
-          fts5ExprAddChildren(pRet, pLeft);
-          fts5ExprAddChildren(pRet, pRight);
+          int cLRet = fts5ExprAddChildren(pRet, pLeft);
+          int cRRet = fts5ExprAddChildren(pRet, pRight);
           if( pRet->iHeight>SQLITE_FTS5_MAX_EXPR_DEPTH ){
             sqlite3Fts5ParseError(pParse, 
                 "fts5 expression tree is too large (maximum depth %d)", 
@@ -2416,6 +2416,9 @@ Fts5ExprNode *sqlite3Fts5ParseNode(
             );
             sqlite3_free(pRet);
             pRet = 0;
+          } else {
+            if (cLRet) sqlite3_free(pLeft);
+            if (cRRet) sqlite3_free(pRight);
           }
         }
       }
-- 
2.45.2

Wouldn't it make more sense to move the va_start inside the if (zFmt) rather than move the va_end outside it?

Wouldn't it make more sense to move ...

Possibly so, but it's really a case of the proverbial "six one way and half-a-dozen the other."

The only places that function is used with a falsy zFmt are allocation errors and a serious internal misuse error (which "cannot happen") when calling sqlite3_bind_value(). As those will never be called during normal operation, that if() block will, in effect, always be entered, so there would seem to be little benefit now to moving the va_list handling into that block.

Thanks for looking into all these.

Issue #1: Possible overflow in ext/fts3/fts3_snippet.c L:401

We check for truncation on L:393 and allow only max int value - 1. Problem is that pIter->nSnippet is capped at values -64 to +64,

I think it's guaranteed to be >=0. Because here:

https://sqlite.org/src/info/610328fe12?ln=1484

Will add an assert().

Issue #3: Accessing freed memory and double free in ext/fts5/fts5_expr.c freeing on L:2410 and accessing on L:2430

I don't think it can, because the case in fts5ExprAddChildren() that does call sqlite3_free does not increase the depth of the expression. Therefore the if(...) condition in the caller cannot be true.

But it's not really good code either. So we'll change it to make it clear that ownership of the children passes to the parent once fts5ExprAddChildren() is called and they are deleted as part of the parent (pRet) in cases where fts5ExprAddChildren() has been called.

Issue #4: Potentially invalid index in ext/expert/sqlite3expert.c L:1517

The assert() is incorrect as you say - should be assert( iSlot<p->nSlot );. Will fix that.

That SQL function is only available to code inside sqlite3expert.c though, so it's safe enough to assert() things about the arguments passed to it.

Thanks again,

Dan.

I think it's guaranteed to be >=0. Because here:

https://sqlite.org/src/info/610328fe12?ln=1484

You are completely right, I was even checking the if statement to make sure the nFToken can't be outside the -64 - +64 range on the true side, but I completely missed that we make it a positive number if it is negative. My apologies.

I don't think it can, because the case in fts5ExprAddChildren() that does call sqlite3_free does not increase the depth of the expression.

Good, I mean I wasn't sure if it did or not, but if it doesn't, that's perfect, and it's another false positive.

Thank you for the code change, it really helps as overall the SQLite codebase is really readable so you are doing a great job.

The assert() is incorrect as you say - should be assert( iSlot

nSlot );. Will fix that.

Shouldn't we also assert assert( iSlot>=0 ); to prevent accidental negative invalid index? It probably shouldn't ever happen by mistake, but I think it might clarify the code.

Also, I am glad for the clarification for whether we can depend just on assert here. I was confused where the argv is coming from.