From 9053c67e95a16f0cb31184ef6a00c11d15c8f801 Mon Sep 17 00:00:00 2001
From: Owen Mansel-Chan <owen-mc@github.com>
Date: Thu, 13 Jun 2024 19:39:22 +0100
Subject: [PATCH] Recognize more non-returning logging functions

---
 go/ql/lib/semmle/go/Concepts.qll              | 31 ++++++++++++-------
 go/ql/lib/semmle/go/frameworks/Logrus.qll     |  6 ++++
 go/ql/lib/semmle/go/frameworks/Zap.qll        |  4 +--
 go/ql/lib/semmle/go/frameworks/stdlib/Log.qll | 10 ++++--
 4 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/go/ql/lib/semmle/go/Concepts.qll b/go/ql/lib/semmle/go/Concepts.qll
index c33fb0ae6bb..f511bb8ac8d 100644
--- a/go/ql/lib/semmle/go/Concepts.qll
+++ b/go/ql/lib/semmle/go/Concepts.qll
@@ -413,17 +413,13 @@ private class ExternalLoggerCall extends LoggerCall::Range, DataFlow::CallNode {
   }
 }
 
-/**
- * A call to an interface that looks like a logger. It is common to use a
- * locally-defined interface for logging to make it easy to changing logging
- * library.
- */
-private class HeuristicLoggerCall extends LoggerCall::Range, DataFlow::CallNode {
-  HeuristicLoggerCall() {
-    exists(Method m, string tp, string logFunctionPrefix, string name |
-      m = this.getTarget() and
-      m.hasQualifiedName(_, tp, name) and
-      m.getReceiverBaseType().getUnderlyingType() instanceof InterfaceType
+private class HeuristicLoggerFunction extends Method {
+  string logFunctionPrefix;
+
+  HeuristicLoggerFunction() {
+    exists(string tp, string name |
+      this.hasQualifiedName(_, tp, name) and
+      this.getReceiverBaseType().getUnderlyingType() instanceof InterfaceType
     |
       tp.regexpMatch(".*[lL]ogger") and
       logFunctionPrefix =
@@ -435,6 +431,19 @@ private class HeuristicLoggerCall extends LoggerCall::Range, DataFlow::CallNode
     )
   }
 
+  override predicate mayReturnNormally() { logFunctionPrefix != "Fatal" }
+
+  override predicate mustPanic() { logFunctionPrefix = "Panic" }
+}
+
+/**
+ * A call to an interface that looks like a logger. It is common to use a
+ * locally-defined interface for logging to make it easy to changing logging
+ * library.
+ */
+private class HeuristicLoggerCall extends LoggerCall::Range, DataFlow::CallNode {
+  HeuristicLoggerCall() { this.getTarget() instanceof HeuristicLoggerFunction }
+
   override DataFlow::Node getAMessageComponent() { result = this.getASyntacticArgument() }
 }
 
diff --git a/go/ql/lib/semmle/go/frameworks/Logrus.qll b/go/ql/lib/semmle/go/frameworks/Logrus.qll
index 33287462c05..069764318d5 100644
--- a/go/ql/lib/semmle/go/frameworks/Logrus.qll
+++ b/go/ql/lib/semmle/go/frameworks/Logrus.qll
@@ -28,6 +28,12 @@ module Logrus {
         this.(Method).hasQualifiedName(packagePath(), ["Entry", "Logger"], name)
       )
     }
+
+    override predicate mayReturnNormally() {
+      not exists(string level, string suffix | level = ["Fatal", "Panic"] |
+        this.getName() = level + suffix
+      )
+    }
   }
 
   private class StringFormatters extends StringOps::Formatting::Range instanceof LogFunction {
diff --git a/go/ql/lib/semmle/go/frameworks/Zap.qll b/go/ql/lib/semmle/go/frameworks/Zap.qll
index b634d8e9795..cf0abcd9336 100644
--- a/go/ql/lib/semmle/go/frameworks/Zap.qll
+++ b/go/ql/lib/semmle/go/frameworks/Zap.qll
@@ -47,7 +47,7 @@ module Zap {
   }
 
   /** A Zap logging function which always panics. */
-  private class FatalLogMethod extends Method {
+  private class FatalLogMethod extends ZapFunction {
     FatalLogMethod() {
       this.hasQualifiedName(packagePath(), "Logger", "Fatal")
       or
@@ -58,7 +58,7 @@ module Zap {
   }
 
   /** A Zap logging function which always panics. */
-  private class MustPanicLogMethod extends Method {
+  private class MustPanicLogMethod extends ZapFunction {
     MustPanicLogMethod() {
       this.hasQualifiedName(packagePath(), "Logger", "Panic")
       or
diff --git a/go/ql/lib/semmle/go/frameworks/stdlib/Log.qll b/go/ql/lib/semmle/go/frameworks/stdlib/Log.qll
index a5ebca68be5..390258591a9 100644
--- a/go/ql/lib/semmle/go/frameworks/stdlib/Log.qll
+++ b/go/ql/lib/semmle/go/frameworks/stdlib/Log.qll
@@ -29,14 +29,20 @@ module Log {
   }
 
   private class LogFormatter extends StringOps::Formatting::Range instanceof LogFunction {
-    LogFormatter() { this.getName() = ["Fatalf", "Panicf", "Printf"] }
+    LogFormatter() { this.getName() = ["Fatalf", "Panicf", "Printf", "Panic", "Panicf", "Panicln"] }
 
     override int getFormatStringIndex() { result = 0 }
   }
 
   /** A fatal log function, which calls `os.Exit`. */
   private class FatalLogFunction extends Function {
-    FatalLogFunction() { this.hasQualifiedName("log", ["Fatal", "Fatalf", "Fatalln"]) }
+    FatalLogFunction() {
+      exists(string fn | fn = ["Fatal", "Fatalf", "Fatalln"] |
+        this.hasQualifiedName("log", fn)
+        or
+        this.(Method).hasQualifiedName("log", "Logger", fn)
+      )
+    }
 
     override predicate mayReturnNormally() { none() }
   }