Repository: llvm/llvm-project
File: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp (C++)

Code before:
  const MemRegion *ArgvReg =
      State->getRegion(FD->parameters()[1], C.getLocationContext());
  SVal ArgvSVal = State->getSVal(ArgvReg);
  State = addTaint(State, ArgvSVal);
  StringRef ArgvName = FD->parameters()[1]->getName();

  bool HaveEnvp = FD->param_size() > 2;
  SVal EnvpSVal;
  StringRef EnvpName;
  if (HaveEnvp && !isPointerToCharArray(FD->parameters()[2]->getType()))
    return;
  if (HaveEnvp) {
    const MemRegion *EnvPReg =
        State->getRegion(FD->parameters()[2], C.getLocationContext());
    EnvpSVal = State->getSVal(EnvPReg);
    EnvpName = FD->parameters()[2]->getName();
    State = addTaint(State, EnvpSVal);
  }

  const NoteTag *OriginatingTag =
      C.getNoteTag([ArgvSVal, ArgcSVal, ArgcName, ArgvName, EnvpSVal,
                    EnvpName](PathSensitiveBugReport &BR) -> std::string {
        // We give diagnostics only for taint related reports
        if ((!BR.isInteresting(ArgcSVal) && !BR.isInteresting(ArgvSVal) &&
             !BR.isInteresting(EnvpSVal)) ||
            BR.getBugType().getCategory() != categories::TaintedData)
          return "";
        std::string Message = "";
        if (BR.isInteresting(ArgvSVal))
          Message += "'" + ArgvName.str() + "'";
        if (BR.isInteresting(ArgcSVal)) {
          if (Message.size() > 0)
            Message += ", ";
          Message += "'" + ArgcName.str() + "'";
        }
        if (BR.isInteresting(EnvpSVal)) {
          if (Message.size() > 0)
            Message += ", ";
          Message += "'" + EnvpName.str() + "'";
        }
        return "Taint originated in " + Message;
      });
  C.addTransition(State, OriginatingTag);
}

void GenericTaintChecker::checkPreCall(const CallEvent &Call,
                                       CheckerContext &C) const {

  initTaintRules(C);

  // FIXME: this should be much simpler.

Review comment (suggestion):
I'd recommend splitting this `if` statement, and bail out earlier if its not a tainted category.
Then you wouldn't need to mix logical-and and logical-or operations in the same condition.
I'd find that a lot more readable.

Code after:
  const MemRegion *ArgvReg =
      State->getRegion(FD->parameters()[1], C.getLocationContext());
  SVal ArgvSVal = State->getSVal(ArgvReg);
  State = addTaint(State, ArgvSVal);
  StringRef ArgvName = FD->parameters()[1]->getName();

  bool HaveEnvp = FD->param_size() > 2;
  SVal EnvpSVal;
  StringRef EnvpName;
  if (HaveEnvp && !isPointerToCharArray(FD->parameters()[2]->getType()))
    return;
  if (HaveEnvp) {
    const MemRegion *EnvPReg =
        State->getRegion(FD->parameters()[2], C.getLocationContext());
    EnvpSVal = State->getSVal(EnvPReg);
    EnvpName = FD->parameters()[2]->getName();
    State = addTaint(State, EnvpSVal);
  }

  const NoteTag *OriginatingTag =
      C.getNoteTag([ArgvSVal, ArgcSVal, ArgcName, ArgvName, EnvpSVal,
                    EnvpName](PathSensitiveBugReport &BR) -> std::string {
        if ((!BR.isInteresting(ArgcSVal) && !BR.isInteresting(ArgvSVal) &&
             !BR.isInteresting(EnvpSVal)))
          return "";
        if (BR.getBugType().getCategory() != categories::TaintedData)
          return "";
        std::string Message = "";
        if (BR.isInteresting(ArgvSVal))
          Message += "'" + ArgvName.str() + "'";
        if (BR.isInteresting(ArgcSVal)) {
          if (Message.size() > 0)
            Message += ", ";
          Message += "'" + ArgcName.str() + "'";
        }
        if (BR.isInteresting(EnvpSVal)) {
          if (Message.size() > 0)
            Message += ", ";
          Message += "'" + EnvpName.str() + "'";
        }
        return "Taint originated in " + Message;
      });
  C.addTransition(State, OriginatingTag);
}

void GenericTaintChecker::checkPreCall(const CallEvent &Call,
                                       CheckerContext &C) const {

  initTaintRules(C);

  // FIXME: this should be much simpler.