Merge pull request #7163 from github/ruby/file-reader-extend

Ruby: Extend `FileSystemReadAccess` to include more potential sources of input from the filesystem

Author: alexrford

ruby/ql/lib/codeql/ruby/frameworks/Files.qll

@@ -34,7 +34,7 @@ private predicate pathArgSpawnsSubprocess(Expr arg) {
 private DataFlow::Node fileInstanceInstantiation() {
   result = API::getTopLevelMember("File").getAnInstantiation()
   or
-  result = API::getTopLevelMember("File").getAMethodCall("open")
+  result = API::getTopLevelMember("File").getAMethodCall(["open", "try_convert"])
   or
   // Calls to `Kernel.open` can yield `File` instances
   result.(KernelMethodCall).getMethodName() = "open" and
@@ -52,22 +52,20 @@ private DataFlow::Node fileInstance() {
   )
 }
 
-private string ioFileReaderClassMethodName() {
-  result = ["binread", "foreach", "read", "readlines", "try_convert"]
-}
+private string ioReaderClassMethodName() { result = ["binread", "foreach", "read", "readlines"] }
 
-private string ioFileReaderInstanceMethodName() {
+private string ioReaderInstanceMethodName() {
   result =
     [
       "getbyte", "getc", "gets", "pread", "read", "read_nonblock", "readbyte", "readchar",
       "readline", "readlines", "readpartial", "sysread"
     ]
 }
 
-private string ioFileReaderMethodName(boolean classMethodCall) {
-  classMethodCall = true and result = ioFileReaderClassMethodName()
+private string ioReaderMethodName(string receiverKind) {
+  receiverKind = "class" and result = ioReaderClassMethodName()
   or
-  classMethodCall = false and result = ioFileReaderInstanceMethodName()
+  receiverKind = "instance" and result = ioReaderInstanceMethodName()
 }
 
 /**
@@ -100,82 +98,94 @@ module IO {
 
   /**
    * A `DataFlow::CallNode` that reads data using the `IO` class. For example,
-   * the `IO.read call in:
+   * the `read` and `readline` calls in:
    *
    * ```rb
+   * # invokes the `date` shell command as a subprocess, returning its output as a string
    * IO.read("|date")
-   * ```
    *
-   * returns the output of the `date` shell command, invoked as a subprocess.
+   * # reads from the file `foo.txt`, returning its first line as a string
+   * IO.new(IO.sysopen("foo.txt")).readline
+   * ```
    *
-   * This class includes reads both from shell commands and reads from the
-   * filesystem. For working with filesystem accesses specifically, see
-   * `IOFileReader` or the `FileSystemReadAccess` concept.
+   * This class includes only reads that use the `IO` class directly, not those
+   * that use a subclass of `IO` such as `File`.
    */
   class IOReader extends DataFlow::CallNode {
-    private boolean classMethodCall;
-    private string api;
+    private string receiverKind;
 
     IOReader() {
-      // Class methods
-      api = ["File", "IO"] and
-      classMethodCall = true and
-      this = API::getTopLevelMember(api).getAMethodCall(ioFileReaderMethodName(classMethodCall))
+      // `IO` class method calls
+      receiverKind = "class" and
+      this = API::getTopLevelMember("IO").getAMethodCall(ioReaderMethodName(receiverKind))
       or
-      // IO instance methods
-      classMethodCall = false and
-      api = "IO" and
+      // `IO` instance method calls
+      receiverKind = "instance" and
       exists(IOInstanceStrict ii |
         this.getReceiver() = ii and
-        this.getMethodName() = ioFileReaderMethodName(classMethodCall)
-      )
-      or
-      // File instance methods
-      classMethodCall = false and
-      api = "File" and
-      exists(File::FileInstance fi |
-        this.getReceiver() = fi and
-        this.getMethodName() = ioFileReaderMethodName(classMethodCall)
+        this.getMethodName() = ioReaderMethodName(receiverKind)
       )
       // TODO: enumeration style methods such as `each`, `foreach`, etc.
     }
 
     /**
-     * Returns the most specific core class used for this read, `IO` or `File`
+     * Gets a string representation of the receiver kind, either "class" or "instance".
      */
-    string getAPI() { result = api }
-
-    predicate isClassMethodCall() { classMethodCall = true }
+    string getReceiverKind() { result = receiverKind }
   }
 
   /**
    * A `DataFlow::CallNode` that reads data from the filesystem using the `IO`
-   * class. For example, the `IO.read call in:
+   * or `File` classes. For example, the `IO.read` and `File#readline` calls in:
    *
    * ```rb
+   * # reads the file `foo.txt` and returns its contents as a string.
    * IO.read("foo.txt")
-   * ```
    *
-   * reads the file `foo.txt` and returns its contents as a string.
+   * # reads from the file `foo.txt`, returning its first line as a string
+   * File.new("foo.txt").readline
+   * ```
    */
-  class IOFileReader extends IOReader, FileSystemReadAccess::Range {
-    IOFileReader() {
-      this.getAPI() = "File"
-      or
-      this.isClassMethodCall() and
-      // Assume that calls that don't invoke shell commands will instead
-      // read from a file.
+  class FileReader extends DataFlow::CallNode, FileSystemReadAccess::Range {
+    private string receiverKind;
+    private string api;
+
+    FileReader() {
+      // A viable `IOReader` that could feasibly read from the filesystem
+      api = "IO" and
+      receiverKind = this.(IOReader).getReceiverKind() and
       not pathArgSpawnsSubprocess(this.getArgument(0).asExpr().getExpr())
+      or
+      api = "File" and
+      (
+        // `File` class method calls
+        receiverKind = "class" and
+        this = API::getTopLevelMember(api).getAMethodCall(ioReaderMethodName(receiverKind))
+        or
+        // `File` instance method calls
+        receiverKind = "instance" and
+        exists(File::FileInstance fi |
+          this.getReceiver() = fi and
+          this.getMethodName() = ioReaderMethodName(receiverKind)
+        )
+      )
+      // TODO: enumeration style methods such as `each`, `foreach`, etc.
     }
 
-    // TODO: can we infer a path argument for instance method calls?
+    // TODO: Currently this only handles class method calls.
+    // Can we infer a path argument for instance method calls?
     // e.g. by tracing back to the instantiation of that instance
     override DataFlow::Node getAPathArgument() {
-      result = this.getArgument(0) and this.isClassMethodCall()
+      result = this.getArgument(0) and receiverKind = "class"
     }
 
     // This class represents calls that return data
     override DataFlow::Node getADataNode() { result = this }
+
+    /**
+     * Returns the most specific core class used for this read, `IO` or `File`
+     */
+    string getAPI() { result = api }
   }
 }
 
@@ -210,7 +220,7 @@ module File {
    *   puts f.read()
    * ```
    */
-  class FileModuleReader extends IO::IOFileReader {
+  class FileModuleReader extends IO::FileReader {
     FileModuleReader() { this.getAPI() = "File" }
   }
 

ruby/ql/test/library-tests/frameworks/files/Files.expected

@@ -36,23 +36,24 @@ ioInstances
 | Files.rb:24:19:24:40 | call to open |
 | Files.rb:35:1:35:56 | ... = ... |
 | Files.rb:35:13:35:56 | call to open |
-fileReaders
+fileModuleReaders
 | Files.rb:7:13:7:32 | call to readlines |
 ioReaders
-| Files.rb:7:13:7:32 | call to readlines | File |
-| Files.rb:20:13:20:25 | call to read | IO |
-| Files.rb:29:12:29:29 | call to read | IO |
-| Files.rb:32:8:32:23 | call to read | IO |
-ioFileReaders
-| Files.rb:7:13:7:32 | call to readlines | File |
-| Files.rb:29:12:29:29 | call to read | IO |
+| Files.rb:20:13:20:25 | call to read |
+| Files.rb:29:12:29:29 | call to read |
+| Files.rb:32:8:32:23 | call to read |
+fileReaders
+| Files.rb:7:13:7:32 | call to readlines |
+| Files.rb:20:13:20:25 | call to read |
+| Files.rb:29:12:29:29 | call to read |
 fileModuleFilenameSources
 | Files.rb:10:6:10:18 | call to path |
 | Files.rb:11:6:11:21 | call to to_path |
 fileUtilsFilenameSources
 | Files.rb:14:8:14:43 | call to makedirs |
 fileSystemReadAccesses
 | Files.rb:7:13:7:32 | call to readlines |
+| Files.rb:20:13:20:25 | call to read |
 | Files.rb:29:12:29:29 | call to read |
 fileNameSources
 | Files.rb:10:6:10:18 | call to path |

ruby/ql/test/library-tests/frameworks/files/Files.ql

@@ -6,11 +6,11 @@ query predicate fileInstances(File::FileInstance i) { any() }
 
 query predicate ioInstances(IO::IOInstance i) { any() }
 
-query predicate fileReaders(File::FileModuleReader r) { any() }
+query predicate fileModuleReaders(File::FileModuleReader r) { any() }
 
-query predicate ioReaders(IO::IOReader r, string api) { api = r.getAPI() }
+query predicate ioReaders(IO::IOReader r) { any() }
 
-query predicate ioFileReaders(IO::IOFileReader r, string api) { api = r.getAPI() }
+query predicate fileReaders(IO::FileReader r) { any() }
 
 query predicate fileModuleFilenameSources(File::FileModuleFilenameSource s) { any() }