Refactor readlink() buffer check to also handle readlinkat()

lib/checkbufferoverrun.cpp

@@ -107,10 +107,10 @@ void CheckBufferOverrun::possibleBufferOverrunError(const Token *tok, const std:
                     "The source buffer is larger than the destination buffer so there is the potential for overflowing the destination buffer.");
 }
 
-void CheckBufferOverrun::possibleReadlinkBufferOverrunError(const Token* tok, const std::string &varname)
+void CheckBufferOverrun::possibleReadlinkBufferOverrunError(const Token* tok, const std::string &funcname, const std::string &varname)
 {
-    const std::string errmsg = "readlink() might return the full size of '" + varname + "'. Lower the supplied size by one.\n"
+    const std::string errmsg = funcname + "() might return the full size of '" + varname + "'. Lower the supplied size by one.\n" +
-                               "readlink() might return the full size of '" + varname + "'. Lower the supplied size by one. "
+                               funcname + "() might return the full size of '" + varname + "'. Lower the supplied size by one. "
                                "If a " + varname + "[len] = '\\0'; statement follows, it will overrun the buffer.";
 
     reportInconclusiveError(tok, Severity::warning, "possibleReadlinkBufferOverrun", errmsg);
@@ -1192,29 +1192,12 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
                 outOfBoundsError(tok->tokAt(4), "snprintf size", true, n, total_size);
         }
 
-        // readlink()
+        // readlink() / readlinkat() buffer usage
-        if (_settings->standards.posix && Token::Match(tok, "readlink ( %any% , %varid% , %num% )", arrayInfo.varid())) {
+        if (_settings->standards.posix) {
-            const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(6));
+            if (Token::Match(tok, "readlink ( %any% , %varid% , %num% )", arrayInfo.varid()))
-            if (total_size > 0 && n > total_size)
+                checkReadlinkBufferUsage(tok, scope_begin, total_size, false);
-                outOfBoundsError(tok->tokAt(4), "readlink() buf size", true, n, total_size);
+            else if(Token::Match(tok, "readlinkat ( %any , %any% , %varid% , %num% )", arrayInfo.varid()))
-
+                checkReadlinkBufferUsage(tok, scope_begin, total_size, true);
-            if (_settings->inconclusive) {
-                // readlink() never terminates the buffer, check the end of the scope for buffer termination.
-                bool found_termination = false;
-                const Token *scope_end = scope_begin->link();
-                for (const Token *tok2 = tok->tokAt(8); tok2 && tok2 != scope_end; tok2 = tok2->next()) {
-                    if (Token::Match(tok2, "%varid% [ %any% ] = 0 ;", tok->tokAt(4)->varId())) {
-                        found_termination = true;
-                        break;
-                    }
-                }
-
-                if (!found_termination) {
-                    bufferNotZeroTerminatedError(tok, tok->tokAt(4)->str(), "readlink");
-                } else if (n == total_size) {
-                    possibleReadlinkBufferOverrunError(tok, tok->tokAt(4)->str());
-                }
-            }
         }
 
         // undefined behaviour: result of pointer arithmetic is out of bounds
@@ -1227,6 +1210,34 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
     }
 }
 
+void CheckBufferOverrun::checkReadlinkBufferUsage(const Token* tok, const Token *scope_begin, const MathLib::bigint total_size, const bool is_readlinkat)
+{
+    unsigned int param_offset = is_readlinkat ? 2 : 0;
+    const std::string funcname = is_readlinkat ? "readlinkat" : "readlink";
+
+    const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(6 + param_offset));
+    if (total_size > 0 && n > total_size)
+        outOfBoundsError(tok->tokAt(4 + param_offset), funcname + "() buf size", true, n, total_size);
+
+    if (!_settings->inconclusive)
+        return;
+
+    // readlink()/readlinkat() never terminates the buffer, check the end of the scope for buffer termination.
+    bool found_termination = false;
+    const Token *scope_end = scope_begin->link();
+    for (const Token *tok2 = tok->tokAt(8 + param_offset); tok2 && tok2 != scope_end; tok2 = tok2->next()) {
+        if (Token::Match(tok2, "%varid% [ %any% ] = 0 ;", tok->tokAt(4 + param_offset)->varId())) {
+            found_termination = true;
+            break;
+        }
+    }
+
+    if (!found_termination) {
+        bufferNotZeroTerminatedError(tok, tok->tokAt(4 + param_offset)->str(), funcname);
+    } else if (n == total_size) {
+        possibleReadlinkBufferOverrunError(tok, funcname, tok->tokAt(4 + param_offset)->str());
+    }
+}
 
 //---------------------------------------------------------------------------
 // Checking local variables in a scope

lib/checkbufferoverrun.h

@@ -195,6 +195,9 @@ public:
     /** Helper function used when parsing for-loops */
     void parse_for_body(const Token *tok2, const ArrayInfo &arrayInfo, const std::string &strindex, bool condition_out_of_bounds, unsigned int counter_varid, const std::string &min_counter_value, const std::string &max_counter_value);
 
+    /** Check readlink or readlinkat() buffer usage */
+    void checkReadlinkBufferUsage(const Token *tok, const Token *scope_begin, const MathLib::bigint total_size, const bool is_readlinkat);
+
     /**
      * Helper function for checkFunctionCall - check a function parameter
      * \param tok token for the function name
@@ -223,7 +226,7 @@ public:
     void pointerOutOfBoundsError(const Token *tok, const std::string &object);	// UB when result of calculation is out of bounds
     void arrayIndexThenCheckError(const Token *tok, const std::string &indexName);
     void possibleBufferOverrunError(const Token *tok, const std::string &src, const std::string &dst, bool cat);
-    void possibleReadlinkBufferOverrunError(const Token *tok, const std::string &varname);
+    void possibleReadlinkBufferOverrunError(const Token *tok, const std::string &funcname, const std::string &varname);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) {
         CheckBufferOverrun c(0, settings, errorLogger);
@@ -241,7 +244,7 @@ public:
         c.pointerOutOfBoundsError(0, "array");
         c.arrayIndexThenCheckError(0, "index");
         c.possibleBufferOverrunError(0, "source", "destination", false);
-        c.possibleReadlinkBufferOverrunError(0, "buffer");
+        c.possibleReadlinkBufferOverrunError(0, "readlink", "buffer");
     }
 
     std::string myName() const {

test/testbufferoverrun.cpp

@@ -236,6 +236,7 @@ private:
 
         TEST_CASE(bufferNotZeroTerminated);
         TEST_CASE(readlink);
+        TEST_CASE(readlinkat);
     }
 
 
@@ -3480,6 +3481,56 @@ private:
               "}\n");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void readlinkat() {
+        check("void f()\n"
+              "{\n"
+              "    int dirfd = 42;\n"
+              "    char buf[255];\n"
+              "    ssize_t len = readlinkat(dirfd, path, buf, sizeof(buf)-1);\n"
+              "    printf(\"%s\n\", buf);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (warning) The buffer 'buf' is not zero-terminated after the call to readlinkat().\n", errout.str());
+
+        check("void f()\n"
+              "{\n"
+              "    int dirfd = 42;\n"
+              "    char buf[255];\n"
+              "    ssize_t len = readlinkat(dirfd, path, buf, sizeof(buf)-1);\n"
+              "    buf[len] = 0;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f()\n"
+              "{\n"
+              "    int dirfd = 42;\n"
+              "    char buf[10];\n"
+              "    ssize_t len = readlinkat(dirf, path, buf, 255);\n"
+              "    buf[len] = 0;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (error) readlinkat() buf size is out of bounds: Supplied size 255 is larger than actual size of 10\n", errout.str());
+
+        check("void f()\n"
+              "{\n"
+              "    int dirfd = 42;\n"
+              "    char buf[255];\n"
+              "    ssize_t len = readlinkat(dirfd, path, buf, sizeof(buf));\n"
+              "    buf[len] = 0;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (warning) readlinkat() might return the full size of 'buf'. Lower the supplied size by one.\n", errout.str());
+
+        check("void f()\n"
+              "{\n"
+              "    int dirfd = 42;\n"
+              "    char buf[255];\n"
+              "    ssize_t len = readlinkat(dirfd, path, buf, sizeof(buf)-1);\n"
+              "    if (len == -1) {\n"
+              "        return;\n"
+              "    }\n"
+              "    buf[len] = 0;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestBufferOverrun)