Fixes from static analysis (#4391)

* Avoid possible silent null-deref in Options::usage * Avoid possible silent null-deref in alias.h * Fix null defer in type checking (after type error) * Avoid null pointer deref in convertErrors * Avoid redundant check for null * Make sure diagnostics don't try to dereference null node just to get its source location * Avoid redundant dynamic cast * Fix tautological condition * Avoid closing file with pclose when not preprocessing * Fix "changes" flag initialization
p4lang · Feb 6, 2024 · 12dafee · 12dafee
1 parent efb20df
commit 12dafee
Show file tree

Hide file tree

Showing 12 changed files with 48 additions and 20 deletions.
diff --git a/frontends/common/constantParsing.cpp b/frontends/common/constantParsing.cpp
@@ -34,7 +34,7 @@ static IR::Constant *parseConstantWithWidth(Util::SourceInfo srcInfo, const char
     char *sep;
     auto size = strtol(text, &sep, 10);
     sep += strspn(sep, " \t\r\n");
-    if (sep == nullptr || !*sep) BUG("Expected to find separator %1%", text);
+    if (!*sep) BUG("Expected to find separator %1%", text);
     if (size < 0) {
         ::error(ErrorType::ERR_INVALID, "%1%: invalid width; %2% must be positive", srcInfo, size);
         return nullptr;

diff --git a/frontends/common/parseInput.h b/frontends/common/parseInput.h
@@ -80,7 +80,11 @@ const IR::P4Program *parseP4File(ParserOptions &options) {
     auto result = options.isv1()
                       ? parseV1Program<FILE *, C>(in, options.file, 1, options.getDebugHook())
                       : P4ParserDriver::parse(in, options.file);
-    options.closeInput(in);
+    if (options.doNotPreprocess) {
+        fclose(in);
+    } else {
+        options.closePreprocessedInput(in);
+    }
 
     if (::errorCount() > 0) {
         ::error(ErrorType::ERR_OVERLIMIT, "%1% errors encountered, aborting compilation",

diff --git a/frontends/common/parser_options.cpp b/frontends/common/parser_options.cpp
@@ -425,13 +425,13 @@ FILE *ParserOptions::preprocess() {
         ssize_t read;
 
         while ((read = getline(&line, &len, in)) != -1) printf("%s", line);
-        closeInput(in);
+        closePreprocessedInput(in);
         return nullptr;
     }
     return in;
 }
 
-void ParserOptions::closeInput(FILE *inputStream) const {
+void ParserOptions::closePreprocessedInput(FILE *inputStream) const {
     if (close_input) {
         int exitCode = pclose(inputStream);
         if (WIFEXITED(exitCode) && WEXITSTATUS(exitCode) == 4)

diff --git a/frontends/common/parser_options.h b/frontends/common/parser_options.h
@@ -80,7 +80,7 @@ class ParserOptions : public Util::Options {
     // Returns the output of the preprocessor.
     FILE *preprocess();
     // Closes the input stream returned by preprocess.
-    void closeInput(FILE *input) const;
+    void closePreprocessedInput(FILE *input) const;
     // True if we are compiling a P4 v1.0 or v1.1 program
     bool isv1() const;
     // Get a debug hook function suitable for insertion

diff --git a/frontends/p4/alias.h b/frontends/p4/alias.h
@@ -123,6 +123,8 @@ class ReadsWrites : public Inspector {
     void postorder(const IR::Operation_Binary *expression) override {
         auto left = ::get(rw, expression->left);
         auto right = ::get(rw, expression->right);
+        CHECK_NULL(left);
+        CHECK_NULL(right);
         rw.emplace(expression, left->join(right));
     }
 
@@ -135,11 +137,13 @@ class ReadsWrites : public Inspector {
 
     void postorder(const IR::Operation_Unary *expression) override {
         auto e = ::get(rw, expression->expr);
+        CHECK_NULL(e);
         rw.emplace(expression, e);
     }
 
     void postorder(const IR::Member *expression) override {
         auto e = ::get(rw, expression->expr);
+        CHECK_NULL(e);
         auto result = e->append(expression->member);
         rw.emplace(expression, result);
     }
@@ -182,18 +186,23 @@ class ReadsWrites : public Inspector {
         auto e0 = ::get(rw, expression->e0);
         auto e1 = ::get(rw, expression->e1);
         auto e2 = ::get(rw, expression->e2);
+        CHECK_NULL(e0);
+        CHECK_NULL(e1);
+        CHECK_NULL(e2);
         rw.emplace(expression, e0->join(e1)->join(e2));
     }
 
     void postorder(const IR::Slice *expression) override {
         auto e = ::get(rw, expression->e0);
+        CHECK_NULL(e);
         rw.emplace(expression, e);
     }
 
     void postorder(const IR::MethodCallExpression *expression) override {
         auto e = ::get(rw, expression->method);
         for (auto a : *expression->arguments) {
             auto s = ::get(rw, a->expression);
+            CHECK_NULL(s);
             e = e->join(s);
         }
         rw.emplace(expression, e);
@@ -203,6 +212,7 @@ class ReadsWrites : public Inspector {
         const SetOfLocations *result = new SetOfLocations();
         for (auto e : *expression->arguments) {
             auto s = ::get(rw, e->expression);
+            CHECK_NULL(s);
             result = result->join(s);
         }
         rw.emplace(expression, result);
@@ -212,6 +222,7 @@ class ReadsWrites : public Inspector {
         const SetOfLocations *result = new SetOfLocations();
         for (auto e : expression->components) {
             auto s = ::get(rw, e->expression);
+            CHECK_NULL(s);
             result = result->join(s);
         }
         rw.emplace(expression, result);
@@ -221,6 +232,7 @@ class ReadsWrites : public Inspector {
         const SetOfLocations *result = new SetOfLocations();
         for (auto e : expression->components) {
             auto s = ::get(rw, e);
+            CHECK_NULL(s);
             result = result->join(s);
         }
         rw.emplace(expression, result);
@@ -237,6 +249,8 @@ class ReadsWrites : public Inspector {
     bool mayAlias(const IR::Expression *left, const IR::Expression *right) {
         auto llocs = get(left);
         auto rlocs = get(right);
+        CHECK_NULL(llocs);
+        CHECK_NULL(rlocs);
         LOG3("Checking overlap between " << llocs << " and " << rlocs);
         return llocs->overlaps(rlocs);
     }

diff --git a/frontends/p4/fromv1.0/programStructure.cpp b/frontends/p4/fromv1.0/programStructure.cpp
@@ -685,7 +685,7 @@ void ProgramStructure::include(cstring filename, cstring ppoptions) {
             auto code = P4::P4ParserDriver::parse(file, options.file);
             if (code && !::errorCount())
                 for (auto decl : code->objects) declarations->push_back(decl);
-            options.closeInput(file);
+            options.closePreprocessedInput(file);
         }
     }
 }

diff --git a/frontends/p4/reassociation.cpp b/frontends/p4/reassociation.cpp
@@ -19,13 +19,13 @@ limitations under the License.
 namespace P4 {
 
 const IR::Node *Reassociation::reassociate(IR::Operation_Binary *root) {
-    auto right = root->right->to<IR::Constant>();
+    const auto *right = root->right->to<IR::Constant>();
     if (!right) return root;
     auto leftBin = root->left->to<IR::Operation_Binary>();
     if (!leftBin) return root;
     if (leftBin->getStringOp() != root->getStringOp()) return root;
     if (!leftBin->right->is<IR::Constant>()) return root;
-    auto c = root->checkedTo<IR::Operation_Binary>()->clone();
+    auto *c = root->clone();
     c->left = leftBin->right;
     c->right = root->right;
     root->left = leftBin->left;

diff --git a/frontends/p4/typeChecking/typeChecker.cpp b/frontends/p4/typeChecking/typeChecker.cpp
@@ -3081,17 +3081,21 @@ const IR::Node *TypeInference::postorder(IR::Slice *expression) {
         auto ei = EnumInstance::resolve(expression->e1, typeMap);
         CHECK_NULL(ei);
         auto sei = ei->to<SerEnumInstance>();
-        if (sei == nullptr)
+        if (sei == nullptr) {
             typeError("%1%: slice bit index values must be constants", expression->e1);
+            return expression;
+        }
         expression->e1 = sei->value;
     }
     auto e2type = getType(expression->e2);
     if (e2type->is<IR::Type_SerEnum>()) {
         auto ei = EnumInstance::resolve(expression->e2, typeMap);
         CHECK_NULL(ei);
         auto sei = ei->to<SerEnumInstance>();
-        if (sei == nullptr)
+        if (sei == nullptr) {
             typeError("%1%: slice bit index values must be constants", expression->e2);
+            return expression;
+        }
         expression->e2 = sei->value;
     }
 
@@ -3430,8 +3434,11 @@ const IR::Expression *TypeInference::actionCall(bool inActionList,
     }
     auto method = actionCall->method;
     auto methodType = getType(method);
-    if (!methodType->is<IR::Type_Action>()) typeError("%1%: must be an action", method);
     auto baseType = methodType->to<IR::Type_Action>();
+    if (!baseType) {
+        typeError("%1%: must be an action", method);
+        return actionCall;
+    }
     LOG2("Action type " << baseType);
     BUG_CHECK(method->is<IR::PathExpression>(), "%1%: unexpected call", method);
     BUG_CHECK(baseType->returnType == nullptr, "%1%: action with return type?",

diff --git a/lib/error_reporter.h b/lib/error_reporter.h
@@ -104,7 +104,7 @@ class ErrorReporter {
         typename... Args>
     void diagnose(DiagnosticAction action, const int errorCode, const char *format,
                   const char *suffix, const T *node, Args... args) {
-        if (!error_reported(errorCode, node->getSourceInfo())) {
+        if (node && !error_reported(errorCode, node->getSourceInfo())) {
             const char *name = get_error_name(errorCode);
             auto da = getDiagnosticAction(name, action);
             if (name)

diff --git a/lib/options.cpp b/lib/options.cpp
@@ -16,6 +16,8 @@ limitations under the License.
 
 #include "options.h"
 
+#include "lib/null.h"
+
 void Util::Options::registerOption(const char *option, const char *argName,
                                    OptionProcessor processor, const char *description,
                                    OptionFlags flags /* = OptionFlags::Default */) {
@@ -114,16 +116,18 @@ void Util::Options::usage() {
     *outStream << binaryName << ": " << message << std::endl;
 
     size_t labelLen = 0;
-    for (auto o : optionOrder) {
+    for (const auto &o : optionOrder) {
         size_t len = o.size();
         auto option = get(options, o);
+        CHECK_NULL(option);
         if (option->argName != nullptr) len += 1 + strlen(option->argName);
         if (labelLen < len) labelLen = len;
     }
 
     labelLen += 3;
-    for (auto o : optionOrder) {
+    for (const auto &o : optionOrder) {
         auto option = get(options, o);
+        CHECK_NULL(option);
         size_t len = strlen(o);
         if (option->flags & OptionFlags::Hide) continue;
         *outStream << option->option;

diff --git a/midend/convertErrors.cpp b/midend/convertErrors.cpp
@@ -50,13 +50,12 @@ const IR::Node *DoConvertErrors::postorder(IR::Type_Name *type) {
 }
 
 const IR::Node *DoConvertErrors::postorder(IR::Member *member) {
-    if (!member->type->is<IR::Type_Error>()) {
+    const auto *typeErr = member->type->to<IR::Type_Error>();
+    if (!typeErr) {
         return member;
     }
-    if (!member->type->is<IR::Type_Error>()) {
-        return member;
-    }
-    auto *r = ::get(repr, member->type->to<IR::Type_Error>()->name);
+    auto *r = ::get(repr, typeErr->name);
+    CHECK_NULL(r);
     if (!member->expr->is<IR::TypeNameExpression>()) {
         // variable
         auto *newMember = member->clone();

diff --git a/midend/removeComplexExpressions.cpp b/midend/removeComplexExpressions.cpp
@@ -104,7 +104,7 @@ const IR::Vector<IR::Expression> *RemoveComplexExpressions::simplifyExpressions(
     // of the list.  Otherwise we simplify the argument itself.
     // This is mostly for functions that take FieldLists - these
     // should still take a list as argument.
-    bool changes = true;
+    bool changes = false;
     auto result = new IR::Vector<IR::Expression>();
     for (auto e : *vec) {
         auto r = simplifyExpression(e, force);