codeql: add _delete and _leave checking queries

contrib/codeql/DoubleDelete.ql

@@ -0,0 +1,38 @@
+/**
+ * @name Double delete
+ * @description Double delete: not all _delete functions are idempotent currently.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id asymmetric-research/double-delete
+ * @tags correctness
+ *       maintainability
+ */
+
+import cpp
+
+import GenericDoubleFree
+import semmle.code.cpp.dataflow.new.DataFlow
+import Flow::PathGraph
+
+bindingset[x]
+string matchDelete(string x) {
+      result = x.regexpCapture("(.*)_delete", 1)
+      and not x = "fd_aio_delete" /* nbridge wants this to behave idempotent */
+}
+
+bindingset[x]
+string matchNew(string x) {
+      result = x.regexpCapture("(.*)_new", 1)
+}
+
+
+module Flow = DataFlow::GlobalWithState<DoubleFreeConfig<matchDelete/1, matchNew/1>>;
+
+from
+  Flow::PathNode source, Flow::PathNode sink
+where
+  Flow::flowPath(source, sink)
+  and source.getLocation().getStartLine() != sink.getLocation().getStartLine()
+  and not source.getLocation().getFile().getBaseName().matches("test%")
+select sink.getNode(), source, sink, "double delete"

contrib/codeql/DoubleLeave.ql

@@ -0,0 +1,36 @@
+/**
+ * @name Double leave
+ * @description Double leave: An object should be joined before being operated on in any way
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id asymmetric-research/double-leave
+ * @tags correctness
+ *       maintainability
+ */
+
+import cpp
+
+import GenericDoubleFree
+import semmle.code.cpp.dataflow.new.DataFlow
+import Flow::PathGraph
+
+bindingset[x]
+string matchLeave(string x) {
+      result = x.regexpCapture("(.*)_leave", 1)
+}
+
+bindingset[x]
+string matchJoin(string x) {
+      result = x.regexpCapture("(.*)_join", 1)
+}
+
+
+module Flow = DataFlow::GlobalWithState<DoubleFreeConfig<matchLeave/1, matchJoin/1>>;
+
+from
+  Flow::PathNode source, Flow::PathNode sink
+where
+  Flow::flowPath(source, sink)
+  and source.getLocation().getStartLine() != sink.getLocation().getStartLine()
+select sink.getNode(), source, sink, "double leave"

contrib/codeql/GenericDoubleFree.qll

@@ -0,0 +1,42 @@
+import cpp
+import semmle.code.cpp.dataflow.new.DataFlow
+
+bindingset[x]
+signature string regcap(string x);
+
+class CandidateCall extends Call {
+  CandidateCall() {
+    not (
+      this.getLocation().getFile().getBaseName().matches("test%") or
+      this.getLocation().getFile().getBaseName().matches("%_ci.%") or
+      this.getLocation().getFile().getBaseName() = "main.c" /* annoying mismatch with funk_init */
+    )
+  }
+}
+
+module DoubleFreeConfig<regcap/1 doubleMatch, regcap/1 barrierMatch> implements
+  DataFlow::StateConfigSig
+{
+  class FlowState = string;
+
+  predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
+    exists(CandidateCall call |
+      source.asIndirectArgument() = call.getArgument(0) and
+      state = doubleMatch(call.getTarget().getName())
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node barrier, DataFlow::FlowState state) {
+    exists(CandidateCall call |
+      barrier.asIndirectArgument() = call.getArgument(0) and
+      state = barrierMatch(call.getTarget().getName())
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
+    exists(CandidateCall call |
+      sink.asIndirectArgument() = call.getArgument(0) and
+      state = doubleMatch(call.getTarget().getName())
+    )
+  }
+}

contrib/codeql/NoMagicCheck.ql

@@ -0,0 +1,60 @@
+/**
+ * As defence in depth for memory corrputions, we should always check and reset the magic field
+ * in a _detroy function. This query finds all functions that have a magic field and do not have
+ * a check for the magic field or a reset of the magic field.
+ * @id asymmetric-research/no-magic-delete
+ * @kind problem
+ * @severity warning
+ */
+
+import cpp
+
+class MagicAccess extends FieldAccess {
+  MagicAccess() { this.getTarget().getName() = "magic" }
+}
+
+class MagicCmp extends ComparisonOperation {
+  MagicCmp() { exists(MagicAccess ma |  this.getAnOperand().getAChild*() = ma) }
+}
+
+class MagicNulling extends Assignment {
+  MagicNulling() { exists(MagicAccess ma | ma.getEnclosingStmt().getAChild*() = this) }
+}
+
+
+class CheckFunction extends Function {
+  Field magicField;
+  Type parentType;
+
+  CheckFunction() {
+    exists(int n | this.getParameter(n)
+                       .getType() = parentType)
+    and
+    parentType.stripType()
+               .(Struct)
+               .getAField() = magicField and
+    magicField.getName() = "magic"
+    and this.getName().regexpCapture("(.*)_(delete|join)", 1) = parentType.getName().regexpCapture("(.*)_t \\*", 1)
+  }
+
+  predicate hasMagicCmp() {
+    exists(MagicCmp cmp | cmp.getBasicBlock().getEnclosingFunction() = this)
+  }
+
+  predicate setsMagicNull() {
+    exists(MagicNulling n | n.getBasicBlock().getEnclosingFunction() = this) 
+  }
+
+  string getParentType() {
+    result = parentType.getName()
+  }
+
+  predicate valid() {
+    this.hasMagicCmp() and 
+    (this.setsMagicNull() or not this.getName().matches("%_delete"))
+  }
+}
+
+from CheckFunction f
+where not f.valid()
+select f, "should check or null magic of " + f.getParentType()

contrib/codeql/NonAnnotatedFormatFunction.ql

@@ -13,4 +13,3 @@ s.getParent().(FunctionCall).getTarget() = f and  /* ignores dynamic function ca
 not exists( Attribute attr | attr = f.getAnAttribute() | attr.getName() = "format" ) and
 f.getLocation().getFile().getRelativePath().regexpMatch("src/.*")
 select f, "Likely format string passed to non-format function"
-

src/ballet/pack/fd_est_tbl.h

@@ -134,6 +134,9 @@ fd_est_tbl_join  ( void         * _tbl ) {
 }
 static inline void         * fd_est_tbl_leave ( fd_est_tbl_t *  tbl ) { return (void         *) tbl; }
 static inline void         * fd_est_tbl_delete( fd_est_tbl_t *  tbl ) {
+  if( FD_UNLIKELY( tbl->magic != FD_EST_TBL_MAGIC ) ) {
+      FD_LOG_WARNING(( "invalid magic!" ));
+  }
   FD_COMPILER_MFENCE();
   FD_VOLATILE( tbl->magic ) = 0UL;
   FD_COMPILER_MFENCE();

src/ballet/zstd/fd_zstd.c

@@ -70,6 +70,9 @@ fd_zstd_dstream_delete( fd_zstd_dstream_t * dstream ) {
 
   if( FD_UNLIKELY( !dstream ) ) return NULL;
 
+  if( FD_UNLIKELY( dstream->magic != FD_ZSTD_DSTREAM_MAGIC ) )
+      FD_LOG_CRIT(( "fd_zstd_dstream_t at %p has invalid magic (memory corruption?)", (void *)dstream ));
+
   /* No need to inform libzstd */
 
   FD_COMPILER_MFENCE();

src/flamenco/snapshot/fd_snapshot_loader.c

@@ -114,7 +114,6 @@ fd_snapshot_loader_delete( fd_snapshot_loader_t * loader ) {
   fd_io_istream_file_delete( loader->vfile );
   fd_snapshot_http_delete  ( loader->http  );
   fd_tar_reader_delete     ( loader->tar   );
-  fd_zstd_dstream_delete   ( loader->zstd  );
 
   if( loader->snapshot_fd>=0 ) {
     if( FD_UNLIKELY( 0!=close( loader->snapshot_fd ) ) )

src/flamenco/vm/jit/fd_jit.c

@@ -91,13 +91,22 @@ fd_jit_prog_new( fd_jit_prog_t *            jit_prog,
      memory was allocated in scratch and is released on function return.  */
   //dasm_free( &d );
 
+  FD_COMPILER_MFENCE();
+  jit_prog->magic = FD_JIT_PROG_MAGIC;
+  FD_COMPILER_MFENCE();
+
   *out_err = FD_VM_SUCCESS;
   return jit_prog;
 }
 
 void *
 fd_jit_prog_delete( fd_jit_prog_t * prog ) {
-  (void)prog;
+  if( FD_UNLIKELY( prog->magic != FD_JIT_PROG_MAGIC ) ) {
+    FD_LOG_WARNING(( "invalid magic!" ));
+  }
+  FD_COMPILER_MFENCE();
+  prog->magic = 0UL;
+  FD_COMPILER_MFENCE();
   return NULL;
 }