Merge pull request #5868 from Marcono1234/marcono1234/ignore-not-closing-char-array-closeable

Java: Ignore char array based closeables for CloseReader.ql and CloseWriter.ql

Author: aschackmull

java/change-notes/2021-05-14-close-resource-leaks-improvements.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The "Potential input resource leak" (`java/input-resource-leak`) and "Potential output resource leak" (`java/output-resource-leak`) queries no longer confuse `java.io` classes such as `Reader` with others that happen to share the same base name. Additionally the number of false positives has been reduced by recognizing `CharArrayReader` and `CharArrayWriter` as types that don't need to be closed.

java/ql/src/Likely Bugs/Resource Leaks/CloseReader.qhelp

@@ -14,7 +14,7 @@ but not closed may cause a resource leak.
 
 <p>Ensure that the resource is always closed to avoid a resource leak. Note that, because of exceptions,
 it is safest to close a resource in a <code>finally</code> block. (However, this is unnecessary for
-subclasses of <code>StringReader</code> and <code>ByteArrayInputStream</code>.)
+subclasses of <code>CharArrayReader</code>, <code>StringReader</code> and <code>ByteArrayInputStream</code>.)
 </p>
 
 <p>For Java 7 or later, the recommended way to close resources that implement <code>java.lang.AutoCloseable</code>

java/ql/src/Likely Bugs/Resource Leaks/CloseReader.ql

@@ -17,16 +17,16 @@ import CloseType
 
 predicate readerType(RefType t) {
   exists(RefType sup | sup = t.getASupertype*() |
-    sup.hasName("Reader") or
-    sup.hasName("InputStream") or
+    sup.hasQualifiedName("java.io", ["Reader", "InputStream"]) or
     sup.hasQualifiedName("java.util.zip", "ZipFile")
   )
 }
 
 predicate safeReaderType(RefType t) {
   exists(RefType sup | sup = t.getASupertype*() |
-    sup.hasName("StringReader") or
-    sup.hasName("ByteArrayInputStream") or
+    sup.hasQualifiedName("java.io", ["CharArrayReader", "StringReader", "ByteArrayInputStream"])
+    or
+    // Note: It is unclear which specific class this is supposed to match
     sup.hasName("StringInputStream")
   )
 }

java/ql/src/Likely Bugs/Resource Leaks/CloseWriter.qhelp

@@ -14,7 +14,7 @@ but not properly closed later may cause a resource leak.
 
 <p>Ensure that the resource is always closed to avoid a resource leak. Note that, because of exceptions,
 it is safest to close a resource properly in a <code>finally</code> block. (However, this is unnecessary for
-subclasses of <code>StringWriter</code> and <code>ByteArrayOutputStream</code>.)</p>
+subclasses of <code>CharArrayWriter</code>, <code>StringWriter</code> and <code>ByteArrayOutputStream</code>.)</p>
 
 <p>For Java 7 or later, the recommended way to close resources that implement <code>java.lang.AutoCloseable</code>
 is to declare them within a <code>try-with-resources</code> statement, so that they are closed implicitly.</p>

java/ql/src/Likely Bugs/Resource Leaks/CloseWriter.ql

@@ -17,15 +17,13 @@ import CloseType
 
 predicate writerType(RefType t) {
   exists(RefType sup | sup = t.getASupertype*() |
-    sup.hasName("Writer") or
-    sup.hasName("OutputStream")
+    sup.hasQualifiedName("java.io", ["Writer", "OutputStream"])
   )
 }
 
 predicate safeWriterType(RefType t) {
   exists(RefType sup | sup = t.getASupertype*() |
-    sup.hasName("StringWriter") or
-    sup.hasName("ByteArrayOutputStream")
+    sup.hasQualifiedName("java.io", ["CharArrayWriter", "StringWriter", "ByteArrayOutputStream"])
   )
 }
 

java/ql/test/query-tests/CloseResource/CloseReader/CloseReader.expected

@@ -1,2 +1,4 @@
-| CloseReader.java:11:42:11:71 | new FileReader(...) | This FileReader is not always closed on method exit. |
-| CloseReader.java:44:6:44:40 | new FileInputStream(...) | This FileInputStream is not always closed on method exit. |
+| CloseReader.java:18:42:18:71 | new FileReader(...) | This FileReader is not always closed on method exit. |
+| CloseReader.java:23:20:23:50 | new FileInputStream(...) | This FileInputStream is not always closed on method exit. |
+| CloseReader.java:33:6:33:40 | new FileInputStream(...) | This FileInputStream is not always closed on method exit. |
+| CloseReader.java:43:21:43:43 | new ZipFile(...) | This ZipFile is not always closed on method exit. |

java/ql/test/query-tests/CloseResource/CloseReader/CloseReader.java

@@ -1,56 +1,73 @@
 import java.io.BufferedReader;
+import java.io.ByteArrayInputStream;
+import java.io.CharArrayReader;
+import java.io.Closeable;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.FileReader;
-import java.io.IOException;
+import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.StringReader;
+import java.util.zip.ZipFile;
 
 class CloseReader {
 
-	public static void test1() throws IOException {
+	void test1() throws IOException {
 		BufferedReader br = new BufferedReader(new FileReader("C:\\test.txt"));
 		System.out.println(br.readLine());
 	}
 
-	public static void test2() throws FileNotFoundException, IOException {
-		BufferedReader br = null;
+	void test2() throws IOException {
+		InputStream in = new FileInputStream("file.bin");
+		in.read();
+	}
+
+	void test3() throws IOException {
+		InputStreamReader reader = null;
 		try {
-			br = new BufferedReader(new FileReader("C:\\test.txt"));
-			System.out.println(br.readLine());
+			// InputStreamReader may throw an exception, in which case the ...
+			reader = new InputStreamReader(
+					// ... FileInputStream is not closed by the finally block
+					new FileInputStream("C:\\test.txt"), "UTF-8");
+			System.out.println(reader.read());
 		}
 		finally {
-			if(br != null)
-				br.close();  // 'br' is closed
+			if (reader != null)
+				reader.close();
 		}
 	}
 
-	public static void test3() throws IOException {
+	void test4() throws IOException {
+		ZipFile zipFile = new ZipFile("file.zip");
+		System.out.println(zipFile.getComment());
+	}
+
+	void testCorrect1() throws IOException {
 		BufferedReader br = null;
 		try {
 			br = new BufferedReader(new FileReader("C:\\test.txt"));
 			System.out.println(br.readLine());
 		}
 		finally {
-			cleanup(br); // 'br' is closed within a helper method
+			if(br != null)
+				br.close();  // 'br' is closed
 		}
 	}
 
-	public static void test4() throws IOException {
-		InputStreamReader reader = null;
+	void testCorrect2() throws IOException {
+		BufferedReader br = null;
 		try {
-			// InputStreamReader may throw an exception, in which case the ...
-			reader = new InputStreamReader(
-					// ... FileInputStream is not closed by the finally block
-					new FileInputStream("C:\\test.txt"), "UTF-8");
-			System.out.println(reader.read());
+			br = new BufferedReader(new FileReader("C:\\test.txt"));
+			System.out.println(br.readLine());
 		}
 		finally {
-			if (reader != null)
-				reader.close();
+			cleanup(br); // 'br' is closed within a helper method
 		}
 	}
 
-	public static void test5() throws IOException {
+	void testCorrect3() throws IOException {
 		FileInputStream fis = null;
 		InputStreamReader reader = null;
 		try {
@@ -66,7 +83,7 @@ public static void test5() throws IOException {
 		}
 	}
 
-	public static void test6() throws IOException {
+	void testCorrect4() throws IOException {
 		BufferedReader br = null;
 		try {
 			br = new BufferedReader(new FileReader("C:\\test.txt"));
@@ -77,15 +94,15 @@ public static void test6() throws IOException {
 		}
 	}
 
-	public static void cleanup(java.io.Closeable... closeables) throws IOException {
-		for (java.io.Closeable c : closeables) {
+	void cleanup(Closeable... closeables) throws IOException {
+		for (Closeable c : closeables) {
 			if (c != null) {
 				c.close();
 			}
 		}
 	}
 
-	public static class LogFile {
+	static class LogFile {
 		private BufferedReader fileRd;
 		LogFile(String path) {
 			FileReader fr = null;
@@ -100,9 +117,21 @@ public static class LogFile {
 		private void init(InputStreamReader reader) {
 			fileRd = new BufferedReader(reader);
 		}
-		public void readStuff() throws java.io.IOException {
+		public void readStuff() throws IOException {
 			System.out.println(fileRd.readLine());
 			fileRd.close();
 		}
 	}
+
+	// Classes which should be ignored
+	void testIgnore() throws IOException {
+		Reader r1 = new CharArrayReader(new char[] {'a'});
+		r1.read();
+
+		Reader r2 = new StringReader("a");
+		r2.read();
+
+		InputStream i1 = new ByteArrayInputStream(new byte[] {1});
+		i1.read();
+	}
 }

java/ql/test/query-tests/CloseResource/CloseReader/CloseReader.qlref

@@ -1 +1 @@
-Likely Bugs/Resource Leaks/CloseReader.ql
+Likely Bugs/Resource Leaks/CloseReader.ql

java/ql/test/query-tests/CloseResource/CloseWriter/CloseWriter.expected

@@ -0,0 +1,3 @@
+| CloseWriter.java:17:42:17:71 | new FileWriter(...) | This FileWriter is not always closed on method exit. |
+| CloseWriter.java:22:22:22:53 | new FileOutputStream(...) | This FileOutputStream is not always closed on method exit. |
+| CloseWriter.java:32:6:32:41 | new FileOutputStream(...) | This FileOutputStream is not always closed on method exit. |

java/ql/test/query-tests/CloseResource/CloseWriter/CloseWriter.java

@@ -0,0 +1,131 @@
+import java.io.BufferedWriter;
+import java.io.ByteArrayOutputStream;
+import java.io.CharArrayWriter;
+import java.io.Closeable;
+import java.io.FileOutputStream;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.io.OutputStreamWriter;
+import java.io.StringWriter;
+import java.io.Writer;
+import java.util.zip.ZipFile;
+
+class CloseWriter {
+
+	void test1() throws IOException {
+		BufferedWriter bw = new BufferedWriter(new FileWriter("C:\\test.txt"));
+		bw.write("test");
+	}
+
+	void test2() throws IOException {
+		OutputStream out = new FileOutputStream("test.bin");
+		out.write(1);
+	}
+
+	void test3() throws IOException {
+		OutputStreamWriter writer = null;
+		try {
+			// OutputStreamWriter may throw an exception, in which case the ...
+			writer = new OutputStreamWriter(
+					// ... FileOutputStream is not closed by the finally block
+					new FileOutputStream("C:\\test.txt"), "UTF-8");
+			writer.write("test");
+		}
+		finally {
+			if (writer != null)
+				writer.close();
+		}
+	}
+
+	void testCorrect1() throws IOException {
+		BufferedWriter bw = null;
+		try {
+			bw = new BufferedWriter(new FileWriter("C:\\test.txt"));
+			bw.write("test");
+		}
+		finally {
+			if(bw != null)
+				bw.close();  // 'bw' is closed
+		}
+	}
+
+	void testCorrect2() throws IOException {
+		BufferedWriter bw = null;
+		try {
+			bw = new BufferedWriter(new FileWriter("C:\\test.txt"));
+			bw.write("test");
+		}
+		finally {
+			cleanup(bw); // 'bw' is closed within a helper method
+		}
+	}
+
+	void testCorrect3() throws IOException {
+		FileOutputStream fos = null;
+		OutputStreamWriter writer = null;
+		try {
+			fos = new FileOutputStream("C:\\test.txt");
+			writer = new OutputStreamWriter(fos);
+			writer.write("test");
+		}
+		finally {
+			if (fos != null)
+				fos.close();  // 'fos' is closed
+			if (writer != null)
+				writer.close();  // 'writer' is closed
+		}
+	}
+
+	void testCorrect4() throws IOException {
+		BufferedWriter bw = null;
+		try {
+			bw = new BufferedWriter(new FileWriter("C:\\test.txt"));
+			bw.write("test");
+		}
+		finally {
+			cleanup(null, bw); // 'bw' is closed within a varargs helper method, invoked with multiple args
+		}
+	}
+
+	void cleanup(Closeable... closeables) throws IOException {
+		for (Closeable c : closeables) {
+			if (c != null) {
+				c.close();
+			}
+		}
+	}
+
+	static class LogFile {
+		private BufferedWriter fileWr;
+		LogFile(String path) {
+			FileWriter fw = null;
+			try {
+				fw = new FileWriter(path);
+			} catch (IOException e) {
+				System.out.println("Error: File not readable: " + path);
+				System.exit(1);
+			}
+			init(fw);
+		}
+		private void init(OutputStreamWriter writer) {
+			fileWr = new BufferedWriter(writer);
+		}
+		public void writeStuff() throws IOException {
+			fileWr.write("test");
+			fileWr.close();
+		}
+	}
+
+	// Classes which should be ignored
+	void testIgnore() throws IOException {
+		Writer w1 = new CharArrayWriter();
+		w1.write("test");
+
+		Writer w2 = new StringWriter();
+		w2.write("test");
+
+		OutputStream o1 = new ByteArrayOutputStream();
+		o1.write(1);
+	}
+}