René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"False Negative: Guards in while loop with break fail to 'control' variable usage outside","articleBody":"Take the following two examples:\r\n\r\n```java\r\nstatic File safe11() throws IOException {\r\n    File temp = null;\r\n    if (temp == null) {\r\n        while (true) {\r\n            temp = File.createTempFile(\"test\", \"directory\");\r\n            if (temp.delete() \u0026\u0026 temp.mkdir()) {\r\n                break;\r\n            }\r\n        }\r\n    }\r\n    return temp;\r\n}\r\n\r\nFile safe12temp;\r\nFile safe12() throws IOException {\r\n    if (safe12temp == null) {\r\n        while (true) {\r\n            safe12temp = File.createTempFile(\"test\", \"directory\");\r\n            if (safe12temp.delete() \u0026\u0026 safe12temp.mkdir()) {\r\n                break;\r\n            }\r\n        }\r\n    }\r\n    return safe12temp;\r\n}\r\n```\r\n\r\nLet's take `File.createTempFile` as a source of taint, and `_.mkdir()` is a guard.\r\n```ql\r\n/**\r\n * All methods on the class `java.io.File` that create directories.\r\n */\r\nclass MethodFileCreatesDirs extends Method {\r\n  MethodFileCreatesDirs() {\r\n    getDeclaringType() instanceof TypeFile and\r\n    hasName([\"mkdir\", \"mkdirs\"])\r\n  }\r\n}\r\n\r\n/**\r\n * An expression that will create a directory without throwing an exception if a file/directory already exists.\r\n */\r\nprivate predicate isNonThrowingDirectoryCreationExpression(Expr expr, MethodAccess creationCall) {\r\n  creationCall.getMethod() instanceof MethodFileCreatesDirs and creationCall.getQualifier() = expr\r\n}\r\n\r\nprivate class DirectoryCreationBarrierGuard extends DataFlow::BarrierGuard {\r\n  DirectoryCreationBarrierGuard() { isNonThrowingDirectoryCreationExpression(_, this) }\r\n\r\n  override predicate checks(Expr e, boolean branch) {\r\n    this.controls(e, branch)\r\n  }\r\n}\r\n```\r\n\r\nRunning a query for `DirectoryCreationBarrierGuard` will find the `safe12temp.mkdir()` and `temp.mkdir()`.\r\nHowever, when quick evaluating the `DirectoryCreationBarrierGuard::checks` method, it will not find that `safe12temp.mkdir()` is a check for ` return safe12temp` and `temp.mkdir()` is a check for `return safe12temp;`.\r\n\r\nInteresting note: as soon as the outer `null` check is removed, codeQL does correctly identify the guard controls the return. For example, this is detected as correctly guarded:\r\n```java\r\nFile safe12temp;\r\nFile safe12() throws IOException {\r\n\twhile (true) {\r\n        safe12temp = File.createTempFile(\"test\", \"directory\");\r\n        if (safe12temp.delete() \u0026\u0026 safe12temp.mkdir()) {\r\n            break;\r\n        }\r\n    }\r\n    return safe12temp;\r\n}\r\n```\r\n\r\nPractical example:\r\n\r\nhttps://github.com/apache/hive/blob/cba74fa91d4035aee2c39dcf929a0ae480609540/ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveKVResultCache.java#L89-L103\r\n\r\nThe above code is flagged as a false positive result when executing this new query: #4473","author":{"url":"https://github.com/JLLeitschuh","@type":"Person","name":"JLLeitschuh"},"datePublished":"2022-03-18T17:23:54.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":2},"url":"https://github.com/8490/codeql/issues/8490"}