Merge pull request #18415 from smowton/smowton/feature/exclude-writer…

…eplace-from-serializable-checks

Java: exclude `writeReplace`-defining classes from `Serializable` check

java/ql/src/Likely Bugs/Serialization/MissingVoidConstructorsOnSerializable.ql

@@ -24,6 +24,16 @@ where
     c.hasNoParameters() and
     not c.isPrivate()
   ) and
+  // Assume if an object replaces itself prior to serialization,
+  // then it is unlikely to be directly deserialized.
+  // That means it won't need to comply with default serialization rules,
+  // such as non-serializable super-classes having a no-argument constructor.
+  not exists(Method m |
+    m = serial.getAMethod() and
+    m.hasName("writeReplace") and
+    m.getReturnType() instanceof TypeObject and
+    m.hasNoParameters()
+  ) and
   serial.fromSource()
 select serial,
   "This class is serializable, but its non-serializable " +

java/ql/src/change-notes/2025-01-06-write-replace-serializable.md

@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* Classes that define a `writeReplace` method are no longer flagged by the `java/missing-no-arg-constructor-on-serializable` query on the assumption they are unlikely to be deserialized using the default algorithm.

java/ql/test/query-tests/MissingVoidConstructorsOnSerializable/MissingVoidConstructorsOnSerializable.expected

@@ -0,0 +1 @@
+| Test.java:12:7:12:7 | A | This class is serializable, but its non-serializable super-class $@ does not declare a no-argument constructor. | Test.java:4:7:4:21 | NonSerializable | NonSerializable |

java/ql/test/query-tests/MissingVoidConstructorsOnSerializable/MissingVoidConstructorsOnSerializable.qlref

@@ -0,0 +1 @@
+Likely Bugs/Serialization/MissingVoidConstructorsOnSerializable.ql

java/ql/test/query-tests/MissingVoidConstructorsOnSerializable/Test.java

@@ -0,0 +1,24 @@
+import java.io.ObjectStreamException;
+import java.io.Serializable;
+
+class NonSerializable { 
+
+  // Has no default constructor
+  public NonSerializable(int x) { }
+
+}
+
+// BAD: Serializable but its parent cannot be instantiated
+class A extends NonSerializable implements Serializable { 
+  public A() { super(1); }
+}
+
+// GOOD: writeReplaces itself, so unlikely to be deserialized
+// according to default rules.
+class B extends NonSerializable implements Serializable { 
+  public B() { super(2); }
+
+  public Object writeReplace() throws ObjectStreamException {
+    return null;
+  }
+}