Проверяем Microsoft Code Contracts

Мы успешно создали и продолжаем развивать анализатор PVS-Studio для языка C/C++. Со временем стало понятно, что многие из реализованных диагностик никак не связаны с конкретным языком программирования, и тогда мы решили попробовать применить свой опыт к другому языку программирования, к C#. В данной статье будет рассказано о проверке проекта Code Contracts от Microsoft с помощью нового C# анализатора.

О проекте MS Code Contracts

Code Contracts предоставляют метод, с помощью которого выражаются предположения о коде программы в .NET приложениях. Контракты принимают форму предусловий, постусловий и инвариантов объекта, и выступают в роли проверенной документации ваших внешних и внутренних API. Контракты используются для того, чтобы улучшить процесс тестирования с помощью проверки во время выполнения программы, позволяя проводить статическую верификацию контрактов и генерацию документации.

Это проект среднего размера (около ~4000 исходных файлов), который активно развивается: содержит много недописанного и некорректно написанного кода. Именно на этом этапе необходимо внедрять статический анализ в процесс разработки.

О новом анализаторе C# кода

Проект Code Contracts проверен с помощью экспериментальной версии PVS-Studio, которая доступна по этой ссылке: http://files.viva64.com/beta/PVS-Studio_setup.exe.

Анализатор в скором времени перестанет быть экспериментальным. Мы планируем выпустить первую релизную версию PVS-Studio с поддержкой С# уже 22.12.2015. При этом анализатор меняет старший номер версии на 6.0.

Ценовая политика не претерпит никаких изменений. Раньше PVS-Studio позволял проверять программы на языках C, C++, C++/CLI, C++/CX. Теперь к этому списку просто добавляется C#.

Результаты проверки

Подготавливая статью о проверке открытого проекта, мы приводим в ней информацию далеко не о всех предупреждениях, которые выдаёт статический анализатор. Поэтому мы рекомендуем авторам проекта самостоятельно выполнить анализ проекта и изучить все выдаваемые анализатором сообщения.

Наиболее опасные найденные места

V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 3. Present: 2. VSServiceProvider.cs 515

void AskToReportError(Exception exn) {
  ....
  var emailBody = new StringBuilder();
  emailBody.AppendLine("Hi Code Contracts user,");
  emailBody.AppendLine();
  ....
  emailBody.AppendLine(
    String.Format(".... {0} {1} Visual Studio {2} Bug Report",
      typeof(VSServiceProvider).Assembly.GetName().Version,
#if DEBUG
                                                 "Debug"
#else
                                                 "Release"
#endif
                                           ));
  ....
}

Функция String.Format() ожидает 3 аргумента, а передано всего 2 аргумента. В этом случае возникает FormatException.

V3014 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. SparseArray.cs 1956

override public string ToString()
{
  StringBuilder str = new StringBuilder();

  for (int i = 0; i < data.Length; i++)
  {
    if (data[i] != null)
    {
      for (int j = 0; j < lastElement[i]; i++)  //<==
      {
        str.AppendFormat("({0},{1})", data[i][j].Index,
                                      data[i][j].Value);
      }
    }
  }

  return str.ToString();
}

Во вложенном цикле не изменяется переменная-счётчик 'j', т.к. вместо 'j++' выполняется изменение счётчика внешнего цикла 'i++'.

Ещё несколько подобных мест:

• V3014 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'k'. Writer.cs 3984
• V3014 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'count_d'. Octagons.cs 509

V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 203, 207. WeakestPreconditionProver.csToSMT2.cs 203

private BoxedExpression DeclareVariable(....)
{
  var tmp = original.ToString().Replace(' ', '_');
  this.Info.AddDeclaration(string.Format("....", tmp, type));
  this.ResultValue = tmp;

  if (type == FLOAT32)       //<==
  {
    types[original] = FloatType.Float32;
  }
  else if (type == FLOAT64)  //<==
  {
    types[original] = FloatType.Float64;
  }

  return original;
}

Анализатор обнаружил два одинаковых условных выражения, из-за чего операторы во втором условии никогда не получат управления. Хотя, на первый взгляд, это не так, мы перейдём к определению констант FLOAT32 и FLOAT64, и увидим следующий код:

private const string FLOAT32 = "(_ FP 11 53)"; // To change!!!
private const string FLOAT64 = "(_ FP 11 53)";

Константы действительно равны! Хотя здесь и есть комментарий, что необходимо произвести замену значения константы FLOAT32, это место легко пропустить в будущем. В развивающихся проектах важно помечать такие места как TODO и регулярно просматривать результаты статического анализа кода.

V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1200, 1210. OutputPrettyCS.cs 1200

public enum TypeConstraint
{
  NONE,
  CLASS,     //<==
  STRUCT,    //<==
  BASECLASS,
}

public void Output(OutputHelper oh)
{
  Contract.Requires(oh != null);

  oh.Output("where ", false);
  mParent.OutputName(oh);
  oh.Output(" : ", false);
  //** base class
  bool comma = false;
  if (mTypeConstraint == TypeConstraint.CLASS)       //<==???
  {
    oh.Output("class", false);
    comma = true;
  }
  else if (mTypeConstraint == TypeConstraint.STRUCT)
  {
    oh.Output("struct", false);
    comma = true;
  }
  else if (mTypeConstraint == TypeConstraint.CLASS)  //<==???
  {
    oh.Output(mClassConstraint, false);
    comma = true;
  }
}

В этом фрагменте кода одинаковые условия более очевидны. Скорее всего, в одном из условий переменную 'mTypeConstraint' хотели сравнить с константой TypeConstraint.BASECLASS, вместо TypeConstraint.CLASS.

V3022 Expression 'c > 'xFFFF'' is always false. Output.cs 685

private static string Encode(string s)
{
  ....
  foreach( char c in s ) {
    if (c == splitC || c == 'n' || c == '\') {
      specialCount++;
    }
    else if (c > 'x7F') {
      if (c > 'xFFFF') specialCount += 9;
      else specialCount += 5;
    }
  }
  ....
}

Выражение «c > 'xFFFF'» никогда не будет истинным и оператор «specialCount += 9» никогда не выполнится. Переменная 'c' имеет тип Char, максимальное значение которого и есть "xFFFF'. Не очень понятно, как должен работать этот код и как его надо исправить. Возможно мы имеем дело с опечаткой или фрагментом кода, позаимствованным из приложения на другом языке. Например, в языке Си/Си++ иногда используются символы разрядностью 32-бита. И там как раз «играют» со значением 0xFFFF. Пример подобного кода:

/* putUTF8 -- write a character to stdout in UTF8 encoding */
static void putUTF8(long c)
{
  if (c <= 0x7F) {         /* Leave ASCII encoded */
    printf("&#%ld;", c);
  } else if (c <= 0x07FF) {     /* 110xxxxx 10xxxxxx */
    putchar(0xC0 | (c >> 6));
    putchar(0x80 | (c & 0x3F));
  } else if (c <= 0xFFFF) {     /* 1110xxxx + 2 */
    putchar(0xE0 | (c >> 12));
    putchar(0x80 | ((c >> 6) & 0x3F));
    putchar(0x80 | (c & 0x3F));
  } else if (c <= 0x1FFFFF) {    /* 11110xxx + 3 */
    putchar(0xF0 | (c >> 18));
    putchar(0x80 | ((c >> 12) & 0x3F));
    putchar(0x80 | ((c >> 6) & 0x3F));
    putchar(0x80 | (c & 0x3F));
  } else if (c <= 0x3FFFFFF) {    /* 111110xx + 4 */
    putchar(0xF8 | (c >> 24));
    putchar(0x80 | ((c >> 18) & 0x3F));
    putchar(0x80 | ((c >> 12) & 0x3F));
    putchar(0x80 | ((c >> 6) & 0x3F));
    putchar(0x80 | (c & 0x3F));
  } else if (c <= 0x7FFFFFFF) {    /* 1111110x + 5 */
    putchar(0xFC | (c >> 30));
    putchar(0x80 | ((c >> 24) & 0x3F));
    putchar(0x80 | ((c >> 18) & 0x3F));
    putchar(0x80 | ((c >> 12) & 0x3F));
    putchar(0x80 | ((c >> 6) & 0x3F));
    putchar(0x80 | (c & 0x3F));
  } else {          /* Not a valid character... */
    printf("&#%ld;", c);
  } 
}

V3008 The 'this.InsideMonitor' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 751, 749. AssertionCrawlerAnalysis.cs 751

private Data(Data state, Variable v)
{
  this.IsReached = state.IsReached;
  this.InsideMonitor = state.InsideMonitor;  //<==
  this.symbols = new List<Variable>(state.symbols) { v };
  this.InsideMonitor = false;                //<==???
}

Очень подозрительно, что некая функция изменяет состояние объекта с помощью значений, переданных через параметры функции и в последним момент перезаписывает значение поля «this.InsideMonitor» константой 'false'. Ранее уже было выполнено присваивание «this.InsideMonitor = state.InsideMonitor».

V3009 It's odd that this method always returns one and the same value of 'true'. LinearEqualities.cs 5262

public bool TryGetFirstAvailableDimension(out int dim)
{
  for (var i = 0; i < map.Length; i++)
  {
    if (!map[i])
    {
      dim = i;
      return true;
    }
  }

  map.Length++;

  dim = map.Length;

  return true;
}

Анализатор обнаружил функцию, которая всегда возвращает одно значение «true». Можно предположить, что при выполнении условия "!map[i]" функция должна вернуть одно значение, а если условие ни разу не было истинным, то вернуть другое значение. Возможно, здесь ошибка.

Остальные предупреждения

V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 1. Present: 2. Output.cs 68

public override void WriteLine(string value)
{
  output.WriteLine(string.Format("{1}", DateTime.Now,
    value.Replace("{", "{{").Replace("}","}}")));
  //output.WriteLine(string.Format("[{0}] {1}",
    //DateTime.Now., value));
}

Раньше функция String.Format() принимала и печатала 2 значения: дату и некоторое значение. Потом этот код закомментировали и написали другой вариант, в котором аргумент с индексом 0 не используется, т.е. дата теперь не печатается.

Другие примеры вызова функций форматирования с неиспользованными аргументами:

• V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 6. Present: 7. CacheModelExtensions.cs 46
• V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 1. Present: 2. CodeFixesInference.cs 1608
• V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 2. Present: 3. ExpressionManipulation.cs 442

V3004 The 'then' statement is equivalent to the 'else' statement. Metadata.cs 2602

private void SerializeFieldRvaTable(....)
{
  ....
  switch (row.TargetSection){
    case PESection.SData:
    case PESection.TLS:
      Fixup fixup = new Fixup();
      fixup.fixupLocation = writer.BaseStream.Position;
      fixup.addressOfNextInstruction = row.RVA;
      if (row.TargetSection == PESection.SData){
        sdataFixup.nextFixUp = fixup;   //<==
        sdataFixup = fixup;             //<==
      }else{
        sdataFixup.nextFixUp = fixup;   //<==
        sdataFixup = fixup;             //<==
      }
      writer.Write((int)0);
      break;
  ....
}

Анализатор обнаружил идентичные блоки кода в условном операторе. Это может быть лишним кодом или один блок кода забыли изменить после копирования. Copy-Paste не щадит и C# программистов.

Весь список подобных мест:

• V3004 The 'then' statement is equivalent to the 'else' statement. Nodes.cs 6698
• V3004 The 'then' statement is equivalent to the 'else' statement. Nodes.cs 6713
• V3004 The 'then' statement is equivalent to the 'else' statement. WarningSuggestionLinkOutput.cs 108
• V3004 The 'then' statement is equivalent to the 'else' statement. NonNullAnalyzer.cs 1753

V3001 There are identical sub-expressions 'semanticType.Name == null' to the left and to the right of the '||' operator. ContractsProvider.cs 694

public bool TryGetTypeReference(....)
{
  ....
  if (semanticType.Name == null || semanticType.Name == null)
    goto ReturnFalse;
  cciType = new Microsoft.Cci.MutableCodeModel.NamespaceTypeR....
  {
    ContainingUnitNamespace = cciNamespace,
    GenericParameterCount = (ushort) (....),
    InternFactory = Host.InternFactory,
    IsValueType = semanticType.IsValueType,
    IsEnum = semanticType.TypeKind == TypeKind.Enum,
    Name = Host.NameTable.GetNameFor(semanticType.Name),
    TypeCode=CSharpToCCIHelper.GetPrimitiveTypeCode(semanticType)
  };
  goto ReturnTrue;'
  ....
}

Условие «semanticType.Name == null» проверяют 2 раза. Либо проверка избыточна и её можно упростить, либо забыли проверить другое поле у объекта.

Ещё одно предупреждение на эту тему:

• V3001 There are identical sub-expressions 'semanticType.Name == null' to the left and to the right of the '||' operator. ContractsProvider.cs 714

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'other', 'right'. CallerInvariant.cs 189

public override Predicate JoinWith(Predicate other)
{
  var right = other as PredicateNullness;
  if (other != null)
  {
    if (this.value == right.value)
    {
      return this;
    }
  }

  return PredicateTop.Value;
}

Анализатор обнаружил потенциальную ошибку, которая может привести к доступу по нулевой ссылке. Необходимо сравнивать с 'null' результат выполнения оператора 'as'.

Если возникнет ситуация, когда объект 'other' не будет нулевым, но его не удастся привести к типу 'PredicateNullness', то возникнет доступ по нулевой ссылке при обращении к «right.value».

Таких сравнений в проекте найдено много, вот весь список:

• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'facts', 'moreRefinedFacts'. SimplePostconditionDispatcher.cs 319
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'objProvenance', 'provenance'. AssertionCrawlerAnalysis.cs 816
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'prev', 'other'. NonRelationalValueAbstraction.cs 1063
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'prev', 'castedPrev'. GenericDomains.cs 1657
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'a', 'right'. LinearEqualitiesForSubpolyhedra.cs 859
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'a', 'other'. NonRelationalValueAbstraction.cs 1047
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'a', 'other'. NonRelationalValueAbstraction.cs 1055
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'a', 'right'. LinearEqualities.cs 849
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'a', 'right'. LinearEqualities.cs 973
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'a', 'right'. LinearEqualities.cs 1119

V3030 Recurring check. The 'this.lineOffsets == null' condition was already verified in line 612. Nodes.cs 613

public virtual void InsertOrDeleteLines(....)
{
  ....
  if (this.lineOffsets == null)
    if (this.lineOffsets == null) this.ComputeLineOffsets();
  if (lineCount < 0)
    this.DeleteLines(offset, -lineCount);
  else
    this.InsertLines(offset, lineCount);
  ....
}

Идущие подряд две одинаковые проверки «this.lineOffsets == null». Такой код не имеет смысла. Возможно, хотели проверить что-то ещё.

V3002 The switch statement does not cover all values of the 'UnaryOperator' enum: Conv_dec. WeakestPreconditionProver.csToSMT2.cs 453

private string Combine(UnaryOperator unaryOperator, string arg)
{
  Contract.Requires(arg != null);

  var format = "({0} {1})";
  string op = null;

  switch (unaryOperator)
  {
    case UnaryOperator.Neg:
    case UnaryOperator.Not:
    case UnaryOperator.Not:
      {
        op = "not";
      }
      break;

    case UnaryOperator.WritableBytes:
    case UnaryOperator.Conv_i:
    case UnaryOperator.Conv_i1:
    case UnaryOperator.Conv_i2:
    case UnaryOperator.Conv_i4:
    case UnaryOperator.Conv_i8:
    case UnaryOperator.Conv_r_un:
    case UnaryOperator.Conv_r4:
    case UnaryOperator.Conv_r8:
    case UnaryOperator.Conv_u:
    case UnaryOperator.Conv_u1:
    case UnaryOperator.Conv_u2:
    case UnaryOperator.Conv_u4:
    case UnaryOperator.Conv_u8:
      {
        return null;
     }
  }

  return string.Format(format, op, arg);
}

Анализатор обнаружил оператор 'switch', в котором выбор варианта осуществляется по переменной enum-типа. При этом в операторе 'switch' пропущен 1 элемент «UnaryOperator. Conv_dec». Это очень подозрительно.

Ниже приведено определение перечисления «UnaryOperator»:

public enum UnaryOperator
{
  ....
  Conv_u8,
  Conv_r_un,
  Neg,
  Not,
  WritableBytes,
  Conv_dec,      //<==
}

Возможная ошибка заключается в том, что данная функция реализована таким образом, чтобы для значения «UnaryOperator.Not» возвращать форматированную строку, а в остальных случаях значение 'null'. Но так как элемент «UnaryOperator. Conv_dec» пропущен, то для него значение переменной 'op' останется равно 'null' и оно попадёт в форматированную строку, которую вернёт функция.

Заключение

Надеемся вам понравилась эта статья. Обещаем и дальше радовать наших читателей проверками интересных открытых проектов.

Как уже упоминалось выше, первый релиз намечен на 22.12.2015. Как правило в конце года принимаются решения о будущих закупках. Поэтому, всем заинтересовавшимся, предлагаем, не откладывая уже сейчас связаться с нами по вопросам приобретения PVS-Studio. И мы будем рады видеть вашу компанию среди наших довольных клиентов.

Спасибо за внимание. И безбажного вам кода!