PHP – компилируемый язык?! PVS-Studio ищет ошибки в PeachPie

• Проблемы с WriteLine
• Одинаковые case в switch
• Иммутабельный DateTime
• Try-методы с нюансами
• Значение аргумента по умолчанию
• Copy-paste передаёт привет
• Просто лишняя проверка в if
• Так может ли там быть null?
• Fatal warning и exceptions
• Снова лишняя проверка? И снова exception!
• Присваивания и переприсваивания
• Эти неизменяемые строки... не изменить
• or throw != or null
• Странный while true
• data == null && throw NullReferenceException
• Не то исключение
• String.Join ни в чём не виноват!
• Маленькое присваивание в большом методе
• null dereference в логическом выражении
• Вот и всё!

PHP широко известен как интерпретируемый язык программирования, использующийся в основном для разработки сайтов. Однако немногие знают, что для PHP есть ещё и компилятор под .NET - PeachPie. Но вот насколько он качественно сделан? Сможет ли статический анализатор найти в этом компиляторе реальные ошибки? Давайте же узнаем!

Давненько не выходили статьи о проверках C#-проектов с помощью PVS-Studio... А ведь нам ещё составлять топ ошибок за 2021 год (топ за 2020 год, кстати, можно глянуть тут)! Что ж, нужно срочно исправляться. Я рад представить вам обзор результатов проверки PeachPie.

Для начала немного расскажу о проверяемом проекте. PeachPie — это современный компилятор языка PHP с открытым исходным кодом и среда выполнения для .NET Framework и .NET. Он построен на платформе компилятора Microsoft Roslyn и основан на проекте Phalanger первого поколения. С июля 2017 года данный проект также входит в .NET Foundation. Исходный код доступен в репозитории на GitHub.

Кстати, наш C#-анализатор тоже широко использует возможности Roslyn, так что можно сказать, что у PeachPie и PVS-Studio есть что-то общее :). Вообще основам работы с Roslyn посвящена целая статья, написанная как раз на основе нашего опыта работы с этой платформой.

Для проведения же проверки PeachPie нужно было установить анализатор, открыть проект в Visual Studio или Rider и запустить анализ, используя плагин PVS-Studio. Подробнее об этом написано в документации.

Проверять такой большой и серьёзный проект было очень интересно. Надеюсь, что мой обзор найденных в PeachPie ошибок вам покажется не менее интересным. Приятного чтения!

Проблемы с WriteLine

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

public static bool mail(....)
{
  // to and subject cannot contain newlines, replace with spaces
  to = (to != null) ? to.Replace("\r\n", " ").Replace('\n', ' ') : "";
  subject = (subject != null) ? subject.Replace("\r\n", " ").Replace('\n', ' ')
                              : "";

  Debug.WriteLine("MAILER",
                  "mail('{0}','{1}','{2}','{3}')",
                  to,
                  subject,
                  message, 
                  additional_headers);

  var config = ctx.Configuration.Core;
  
  ....
}

Предупреждение V3025: Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: 1st, 2nd, 3rd, 4th, 5th. Mail.cs 25

Казалось бы, а что же тут не так? Вроде бы всё выглядит нормально. Хотя, постойте-ка! А каким по порядку аргументом нужно передавать формат?

Что ж, давайте взглянем на объявление Debug.WriteLine:

public static void WriteLine(string format, params object[] args);

Получается, что строка формата должна передаваться первым аргументом, а в коде первым аргументом является "MAILER". Очевидно, разработчик перепутал методы и передал аргументы некорректным образом.

Одинаковые case в switch

Очередной раздел посвящается срабатываниям, связанным с выполнением одинаковых действий в разных case-ветках:

private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.DisallowNullAttribute":
        result |= FlowAnalysisAnnotations.DisallowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullAttribute":
        result |= FlowAnalysisAnnotations.MaybeNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute":
        if (TryGetBoolArgument(attr, out bool maybeNullWhen))
        {
          result |= maybeNullWhen ? FlowAnalysisAnnotations.MaybeNullWhenTrue
                                  : FlowAnalysisAnnotations.MaybeNullWhenFalse;
        }
        break;
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
    }
  }
}

В данном фрагменте закралась если не ошибка, то как минимум странность. Как быстро вы сможете её найти?

Впрочем, ни к чему терять время, анализатор всё нашёл за нас:

private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      ....
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;              // <=
        break;
    }
  }
}

Предупреждение V3139: Two or more case-branches perform the same actions. ReflectionUtils.Nullability.cs 170

Не правда ли, странно, что два разных случая обрабатываются одинаково? На самом деле нет, в принципе такое бывает достаточно часто. Однако есть 2 "но".

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

switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
}

Тем не менее, разработчики нередко пренебрегают этим удобным способом, предпочитая копипаст. Из-за этого наличие двух одинаковых веток само по себе не кажется таким уж страшным. Куда более подозрительным выглядит тот факт, что перечисление FlowAnalysisAnnotations имеет среди прочих значение FlowAnalysisAnnotations.NotNull. Складывается впечатление, что именно оно должно было использоваться при обработке значения "System.Diagnostics.CodeAnalysis.NotNullAttribute":

switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.NotNull;              // <=
    break;
}

Иммутабельный DateTime

Разработчики частенько допускают ошибки, связанные с неверным пониманием особенностей работы "изменяющих" методов. Ниже же представлена ошибка, найденная в PeachPie:

using System_DateTime = System.DateTime;

internal static System_DateTime MakeDateTime(....) { .... }

public static long mktime(....)
{
  var zone = PhpTimeZone.GetCurrentTimeZone(ctx);
  var local = MakeDateTime(hour, minute, second, month, day, year);

  switch (daylightSaving)
  {
    case -1:
      if (zone.IsDaylightSavingTime(local))
        local.AddHours(-1);                   // <=
      break;
    case 0:
      break;
    case 1:
      local.AddHours(-1);                     // <=
      break;
    default:
      PhpException.ArgumentValueNotSupported("daylightSaving", daylightSaving);
      break;
  }
  return DateTimeUtils.UtcToUnixTimeStamp(TimeZoneInfo.ConvertTime(local, 
                                                                   ....));
}

Предупреждения PVS-Studio:

• V3010 The return value of function 'AddHours' is required to be utilized. DateTimeFunctions.cs 1232
• V3010 The return value of function 'AddHours' is required to be utilized. DateTimeFunctions.cs 1239

Анализатор сообщает, что результаты вызовов должны куда-нибудь записываться – в противном случае они попросту не имеют никакого смысла. Дело в том, что методы вроде AddHours не меняют исходный объект – вместо этого возвращается новый, отличающийся от исходного соответствующим образом. Сложно сказать, насколько критична эта оплошность, но очевидно, что код работает некорректно.

Try-методы с нюансами

Try-методы очень часто бывают удобны при разработке приложений на C#. Наиболее известными их представителями являются int.TryParse, Dictionary.TryGetValue и т.д. Привычно, что эти методы возвращают флаг, указывающий на успешность выполнения операции, а сам результат записывается в out-параметр. Разработчики PeachPie решили реализовать свои try-методы, которые, казалось бы, должны были работать по той же схеме. Что же из этого вышло? Давайте взглянем на следующий код:

internal static bool TryParseIso8601Duration(string str,
                                             out DateInfo result,
                                             out bool negative)
{
  ....
  if (pos >= length) goto InvalidFormat;

  if (s[pos++] != 'P') goto InvalidFormat;

  if (!Core.Convert.TryParseDigits(....))
    goto Error;
  
  if (pos >= length) goto InvalidFormat;

  if (s[pos] == 'Y')
  {
    ....

    if (!Core.Convert.TryParseDigits(....)) 
      goto Error;

    if (pos >= length) goto InvalidFormat;
  }
  ....
  InvalidFormat:
  Error:

    result = default;
    negative = default;
    return false;
}

Данный метод сильно сокращён для удобства восприятия. При желании вы можете посмотреть его полностью, перейдя по ссылке. В методе множество раз производится вызов Core.Convert.TryParseDigits. В случаях когда такой вызов возвращает false, поток выполнения переходит к метке Error, что вполне логично.

На метке Error out-параметрам присваиваются значения по умолчанию, после чего метод возвращает false. Всё выглядит вполне логично – метод TryParseIso8601Duration ведёт себя именно так, как и стандартные try-методы. Ну... По крайней мере, это так выглядит. Фактически же всё совсем не так :(.

Как уже было сказано ранее, если Core.Convert.TryParseDigits возвращает false, то код переходит на метку Error, где и производится обработка ошибки/неудачи. Правда, вот в чём беда – анализатор-то сообщает, что TryParseDigits никогда не возвращает false:

Предупреждение V3022: Expression '!Core.Convert.TryParseDigits(s, ref pos, false, out value, out numDigits)' is always false. DateTimeParsing.cs 1440

Если отрицание результата вызова всегда равно false, то сам вызов всегда возвращает true. Достаточно специфичное поведение для try-метода! Неужели операция и правда всегда завершается успешно? Давайте, наконец, взглянем на TryParseDigits:

public static bool TryParseDigits(....)
{
  Debug.Assert(offset >= 0);

  int offsetStart = offset;
  result = 0;
  numDigits = 0;

  while (....)
  {
    var digit = s[offset] - '0';

    if (result > (int.MaxValue - digit) / 10)
    {
      if (!eatDigits)
      {
        // overflow
        //return false;
        throw new OverflowException();
      }

      ....

      return true;
    }

    result = result * 10 + digit;
    offset++;
  }

  numDigits = offset - offsetStart;
  return true;
}

Метод действительно всегда возвращает true. Но операция может и завершиться неудачей – в этом случае будет выброшено исключение типа OverflowException. Как по мне, это явно не то поведение, которого ожидаешь от try-метода :). Строка с return false, кстати, тут есть, но она закомментирована.

Возможно, использование в этом месте исключения как-то обосновано. Но по коду складывается впечатление, что тут что-то пошло не по плану. И TryParseDigits, и использующий его TryParseIso8601Duration, по идее, должны работать как привычные try-методы – возвращать в случае неудачи false. Вместо этого они выбрасывают неожиданные исключения.

Значение аргумента по умолчанию

Следующее срабатывание анализатора попроще, однако и оно указывает на весьма странный фрагмент кода:

private static bool Put(Context context,
                        PhpResource ftp_stream,
                        string remote_file,
                        string local_file,
                        int mode,
                        bool append,
                        int startpos)
{ .... }

public static bool ftp_put(Context context,
                           PhpResource ftp_stream,
                           string remote_file,
                           string local_file,
                           int mode = FTP_IMAGE,
                           int startpos = 0)
{
    return Put(context,
               ftp_stream,
               remote_file,
               local_file,
               mode = FTP_IMAGE, // <=
               false,
               startpos);
}

Предупреждение V3061: Parameter 'mode' is always rewritten in method body before being used. Ftp.cs 306

Метод ftp_put принимает на вход ряд параметров, одним из которых является mode. Данный параметр имеет значение по умолчанию, однако при вызове, очевидно, можно задать и другое значение. Правда, это ни на что не повлияет – mode всегда перезаписывается, и в метод Put всегда передаётся значение константы FTP_IMAGE.

Сложно сказать, почему всё написано именно так – конструкция кажется бессмысленной. Скорее всего, здесь всё же допущена ошибка.

Copy-paste передаёт привет

Следующий фрагмент кода похож на жертву копирования:

public static PhpValue filter_var(....)
{
  ....
  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_PRIV_RANGE)
  {
    throw new NotImplementedException();
  }

  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_RES_RANGE)
  {
    throw new NotImplementedException();
  }
  ....
}

Предупреждение V3127: Two similar code fragments were found. Perhaps, this is a typo and 'NO_RES_RANGE' variable should be used instead of 'NO_PRIV_RANGE' Filter.cs 771

Складывается впечатление, что второе условие нужно было записать так:

(flags & (int)FilterFlag.NO_RES_RANGE) == (int)FilterFlag.NO_RES_RANGE

Во всяком случае, такой вариант выглядит логичнее и не вызывает вопросов.

Просто лишняя проверка в if

Думаю, можно разбавить наш сегодняшний разбор и обычным избыточным кодом:

internal static NumberInfo IsNumber(....)
{
  ....
  int num = AlphaNumericToDigit(c);

  // unexpected character:
  if (num <= 15)
  {
    if (l == -1)
    {
      if (   longValue < long.MaxValue / 16 
          || (   longValue == long.MaxValue / 16 
              && num <= long.MaxValue % 16))         // <=
      {
        ....
      }
      ....
    }
    ....
  }
  ....
}

Предупреждение V3063: A part of conditional expression is always true if it is evaluated: num <= long.MaxValue % 16. Conversions.cs 994

В первую очередь хочется сказать, что код функции очень сильно урезан для удобства восприятия. Полностью исходный код IsNumber доступен по ссылке – но хочу предупредить, что изучать её будет непросто, ибо функция содержит более 300 строк кода. Кажется, что она слегка выходит за принятые рамки "одного экрана" :).

Перейдём к срабатыванию. Во внешнем блоке производится проверка, что значение переменной num меньше или равно 15. Во внутреннем же есть проверка, что num меньше или равно long.MaxValue % 16. При этом значение этого выражения равно 15 – это нетрудно проверить. Выходит, код дважды проверяет, что num меньше или равен 15.

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

Так может ли там быть null?

Довольно часто разработчики упускают проверки на null. Особенно интересна ситуация, когда в одном месте функции переменную проверили, а в другом (где она всё так же может быть null) – забыли или не посчитали нужным. И тут остаётся лишь гадать, была ли проверка лишней или напротив – кое-где её не хватает. Проверки на null не всегда предполагают использование операторов сравнения – например, в коде ниже разработчик использовал null- conditional оператор:

public static string get_parent_class(....)
{
  if (caller.Equals(default))
  {
    return null;
  }

  var tinfo = Type.GetTypeFromHandle(caller)?.GetPhpTypeInfo();
  return tinfo.BaseType?.Name;
}

Предупреждение V3105: The 'tinfo' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. Objects.cs 189

По мнению разработчика, вызов Type.GetTypeFromHandle(caller) может вернуть null – оттого он и использовал "?." для вызова GetPhpTypeInfo. Судя по документации, это действительно возможно.

Ура, "?." спасает от одного исключения. Если вызов GetTypeFromHandle действительно вернёт null, то в переменную tinfo также будет записан null. Однако при попытке обращения к свойству BaseType будет выброшено другое исключение. Скорее всего, в последней строке не хватает ещё одного "?":

return tinfo?.BaseType?.Name;

Fatal warning и exceptions

Приготовьтесь, в этом разделе вас ждёт настоящее расследование...

Ещё одно срабатывание, связанное с проверкой на null, оказалось куда интереснее, чем выглядело на первый взгляд. Взгляните на код:

static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context));
  }

  return context.HashAlgorithm;
}

Предупреждение V3125: The 'context' object was used after it was verified against null. Check lines: 3138, 3133. Hash.cs 3138

Действительно, переменная проверяется на равенство null, а затем без какой-либо проверки производится обращение к свойству. Однако посмотрите, что произойдёт, если переменная будет равна null:

PhpException.ArgumentNull(nameof(context));

Получается, что если context и правда будет равен null, то до обращения к свойству HashAlgorithm поток выполнения не доберётся. Следовательно, данный код безопасен. Получается, ложное срабатывание?

Конечно, анализатор может и ошибаться. Однако мне было известно, что PVS-Studio подобные ситуации обрабатывать умеет – анализатор должен был знать, что в момент обращения к HashAlgorithm переменная context не может быть равна null.

А всё-таки что именно делает вызов PhpException.ArgumentNull? Давайте взглянем:

public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}

Хм, действительно, вроде что-то выбрасывается. Обратите внимание на первый аргумент вызова — PhpError.Warning. Хм, ну что ж, перейдём к самому методу Throw:

public static void Throw(PhpError error, string formatString, string arg0)
{
  Throw(error, string.Format(formatString, arg0));
}

Тут в принципе ничего интересного, перейдём в другую перегрузку Throw:

public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}

А вот и то, что мы искали! Получается, что под капотом всей этой системы производится выбрасывание PhpFatalErrorException. Вот только похоже, что выбрасывается исключение не всегда.

В первую очередь стоит поглядеть на места, где регистрируются обработчики события OnError. Всё же они тоже могут кидать исключения – это было бы слегка неожиданно, но мало ли. Таких оказалось немного и все они связаны с логированием соответствующих сообщений. Один был в файле PhpHandlerMiddleware:

PhpException.OnError += (error, message) =>
{
  switch (error)
  {
    case PhpError.Error:
      logger.LogError(message);
      break;

    case PhpError.Warning:
      logger.LogWarning(message);
      break;

    case PhpError.Notice:
    default:
      logger.LogInformation(message);
      break;
  }
};

Другие 2 определялись внутри самого класса PhpException:

// trace output
OnError += (error, message) =>
{
  Trace.WriteLine(message, $"PHP ({error})");
};

// LogEventSource
OnError += (error, message) =>
{
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    LogEventSource.Log.HandleFatal(message);
  }
  else
  {
    LogEventSource.Log.HandleWarning(message);
  }
};

Таким образом, никаких исключений обработчики события не генерируют. Поэтому вернёмся к методу Throw.

public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}

Раз с OnError всё ясно, то давайте подробнее разберём это условие:

(error & (PhpError)PhpErrorSets.Fatal) != 0

Параметр error хранит значение перечисления PhpError. Чуть ранее мы видели, что в этот параметр передаётся PhpError.Warning. Исключение же будет выброшено в случае, если результат применения "побитового и" к error и PhpErrorSets.Fatal не будет нулевым.

Значение PhpErrorSets.Fatal представляет собой "объединение" набора элементов перечисления PhpError с помощью операции "побитового или":

Fatal =   PhpError.E_ERROR | PhpError.E_COMPILE_ERROR
        | PhpError.E_CORE_ERROR | PhpError.E_USER_ERROR

Ниже представлены значения всех рассмотренных ранее элементов перечисления:

E_ERROR = 1,
E_WARNING = 2,
E_CORE_ERROR = 16,
E_COMPILE_ERROR = 64,
E_USER_ERROR = 256,
Warning = E_WARNING

Получается, операция error & (PhpError)PhpErrorSets.Fatal вернёт ненулевое значение только в случае, если параметр error будет иметь одно из следующих значений или их сочетаний:

PhpError.E_ERROR,
PhpError.E_COMPILE_ERROR,
PhpError.E_CORE_ERROR,
PhpError.E_USER_ERROR

Если в параметр error записано значение PhpError.Warning, равное PhpError.E_WARNING, то результат операции "побитового и" будет нулевым. Тогда и условие выбрасывания исключения PhpFatalErrorException не будет выполнено.

Вернёмся назад к методу PhpException.ArgumentNull:

public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}

Мы разобрались, что при передаче PhpError.Warning исключение выброшено не будет. Возможно, по задумке исключение действительно не должно выбрасываться в случаях, когда передан неожиданный null. Вот только...

static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context)); // no exceptions
  }

  return context.HashAlgorithm; // context is potential null
}

Если PhpException.ArgumentNull не выбрасывает исключение (что само по себе уже очень неожиданно), то при обращении к свойству HashAlgorithm всё равно будет выбрасываться NullReferenceException!

Возникает вопрос – должно ли всё-таки выбрасываться исключение или нет? Если должно, то логичнее было бы использовать тот же PhpFatalErrorException. Если же исключения в этой ситуации никто не ожидает, то нужно корректно обработать значение null параметра context. К примеру, использовать "?.". Так или иначе, анализатор всё-таки отработал корректно и даже помог разобраться в этой странной ситуации.

Снова лишняя проверка? И снова exception!

Прошлый случай показывает, как можно ожидать исключение, а нарваться на неожиданный null. Фрагмент ниже – противоположность того случая:

public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    return node.Value;
  else
    return PhpValue.Null;
}

Предупреждение V3022: Expression 'node != null' is always true. Datastructures.cs 432

Ну, как говорится, не null и ладно – чего бухтеть-то? Вот только обычно null ожидается в тех случаях, когда что-то пошло не так. По коду видно, что здесь именно такая ситуация. Анализатор же утверждает, что никакого null тут быть не может.

Можно подумать, что здесь дело в вызове Debug.Assert. Хорошо это или плохо, но на самом деле этот вызов не влияет на мнение анализатора.

Если дело не в Debug.Assert, то в чём же? С чего анализатор считает, что node никогда не равно null? Давайте взглянем на метод GetNodeAtIndex, который и возвращает значение, записанное в node:

private LinkedListNode<PhpValue> GetNodeAtIndex(PhpValue index)
{
  return GetNodeAtIndex(GetValidIndex(index));
}

Что же, копаем дальше. Поглядим на вызванный здесь GetNodeAtIndex:

private LinkedListNode<PhpValue> GetNodeAtIndex(long index)
{
  var node = _baseList.First;
  while (index-- > 0 && node != null)
  {
    node = node.Next;
  }

  return node ?? throw new OutOfRangeException();
}

Глядите – похоже, метод действительно мог бы вернуть null... Но не тут-то было – если после завершения работы цикла node будет равно null, то производится выбрасывание исключения. Таким образом, никакого null возвращено быть не может.

Получается, что в случае возникновения непредвиденной ситуации метод GetNodeAtIndex не вернёт null, как ожидалось в коде метода offsetGet:

public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset); // potential null expected

  Debug.Assert(node != null);

  if (node != null) // always true
    return node.Value;
  else
    return PhpValue.Null; // unreachable
}

При просмотре этого метода любой разработчик может легко обмануться. Ведь по коду складывается впечатление, что либо будет возвращено корректное значение, либо PhpValue.Null. Фактически же этот метод может выбросить исключение.

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

Аналогичная проблема присутствует, кстати, и в методе offsetSet из того же класса:

public void offsetSet(PhpValue offset, PhpValue value)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    node.Value = value;
}

Предупреждение V3022: Expression 'node != null' is always true. Datastructures.cs 444

Присваивания и переприсваивания

Предлагаю немного отдохнуть от всех этих расследований и выпить кружечку кофе.

Пока пьём кофе, рассмотрим более простое срабатывание, которое тем не менее указывает на очень странный фрагмент кода:

internal StatStruct(Mono.Unix.Native.Stat stat)
{
  st_dev = (uint)stat.st_dev;
  st_ctime = stat.st_ctime_nsec;
  st_mtime = stat.st_mtime_nsec;
  st_atime = stat.st_atime_nsec;
  st_ctime = stat.st_ctime;
  st_atime = stat.st_atime;
  //stat.st_blocks;
  //stat.st_blksize;
  st_mtime = stat.st_mtime;
  st_rdev = (uint)stat.st_rdev;
  st_gid = (short)stat.st_gid;
  st_uid = (short)stat.st_uid;
  st_nlink = (short)stat.st_nlink;
  st_mode = (FileModeFlags)stat.st_mode;
  st_ino = (ushort)stat.st_ino;
  st_size = stat.st_size;
}

Предупреждения PVS-Studio:

• V3008 The 'st_ctime' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 78, 75. StatStruct.cs 78
• V3008 The 'st_atime' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 79, 77. StatStruct.cs 79

Выглядит так, будто разработчик запутался во всех этих присваиваниях и где-то опечатался. Это привело к тому, что значения присваиваются полям st_ctime и st_atime по 2 раза – и ведь второй раз присваивается не то же самое значение, что в первый.

Вроде бы ошибка, верно? Но так ведь совсем не интересно! Предлагаю вам потренировать свои навыки поиска глубинного смысла и в комментариях предложить какое-нибудь объяснение того, зачем всё написано именно таким образом.

Ну а пока идём дальше :)

Эти неизменяемые строки... не изменить

Давным-давно, когда вы ещё только просматривали первые срабатывания из этой статьи, мы упоминали неизменяемость экземпляров структуры DateTime. Следующие же предупреждения напомнят нам об аналогичном свойстве строк:

public TextElement Filter(IEncodingProvider enc,
                          TextElement input,
                          bool closing)
{
  string str = input.AsText(enc.StringEncoding);

  if (pending)
  {
    if (str.Length == 0) str = "\r";
    else if (str[0] != '\n') str.Insert(0, "\r"); // <=
  }

  str = str.Replace("\r\n", "\n");
  if (str.Length != 0)
  {
    pending = str[str.Length - 1] == '\r';

    if (!closing && pending) str.Remove(str.Length - 1, 1); // <=
  }

    
  return new TextElement(str);
}

Предупреждения PVS-Studio:

• V3010 The return value of function 'Insert' is required to be utilized. Filters.cs 150
• V3010 The return value of function 'Remove' is required to be utilized. Filters.cs 161

В принципе, всё просто и понятно – хотели внести в строку изменения, но как-то... не срослось :(.

or throw != or null

Недавно мы разбирали случай, когда разработчик ожидал от функции null, а получил исключение. Ниже представлено нечто похожее, но попроще:

public static bool stream_wrapper_register(....)
{
  // check if the scheme is already registered:
  if (   string.IsNullOrEmpty(protocol)
      || StreamWrapper.GetWrapperInternal(ctx, protocol) == null)
  {
    // TODO: Warning?
    return false;
  }

  var wrapperClass = ctx.GetDeclaredTypeOrThrow(classname, true);
  if (wrapperClass == null) // <=
  {
    return false;
  }

  ....
}

Предупреждение V3022: Expression 'wrapperClass == null' is always false. Streams.cs 555

Конечно, можно провести подробный разбор, но... Тут и по названию метода ведь всё видно! GetDeclaredTypeOrThrow как бы намекает, что если что не так, то он будет стрелять. И ведь смотрите опять какая штука – это поведение будет передаваться и методу stream_wrapper_register, который по задумке должен был бы просто вернуть false. Ага, конечно, ловите исключение!

Вообще, мы уже сталкивались ранее с обманывающими названиями. Помните, как вызов метода PhpException.ArgumentNull на самом деле не бросал исключение? Поэтому давайте всё-таки проверим, действительно ли GetDeclaredTypeOrThrow бросает исключение:

PhpTypeInfo GetDeclaredTypeOrThrow(string name, bool autoload = false)
{
  return GetDeclaredType(name, autoload) ??
         throw PhpException.ClassNotFoundException(name);
}

Ну, здесь не обманули – и правда исключение :).

Странный while true

Бывают случаи, когда разработчики используют в качестве условия продолжения цикла while значение true. Кажется, что это в целом нормальная практика – для выхода из цикла может использоваться break, return, ну или те же исключения. Куда более странно выглядит цикл, в качестве условия которого используется не ключевое слово true, а некоторое выражение, значение которого всегда имеет значение true:

public static int stream_copy_to_stream(...., int offset = 0)
{
  ....
  if (offset > 0)
  {
    int haveskipped = 0;

    while (haveskipped != offset)  // <=
    {
      TextElement data;

      int toskip = offset - haveskipped;
      if (toskip > from.GetNextDataLength())
      {
        data = from.ReadMaximumData();
        if (data.IsNull) break;
      }
      else
      {
        data = from.ReadData(toskip, false);
        if (data.IsNull) break; // EOF or error.
        Debug.Assert(data.Length <= toskip);
      }

      Debug.Assert(haveskipped <= offset);
    }
  }
  ....
}

Предупреждение V3022: Expression 'haveskipped != offset' is always true. Streams.cs 769

Переменная haveskipped объявлена перед запуском цикла и инициализирована значением 0. Это значение останется с ней... до самой смерти. Мрачновато вышло, но как есть. По сути, haveskipped — это константа. Значение параметра offset во время выполнения цикла также не меняется. Да и вообще не меняется ни в одном месте функции, на самом деле (можете проверить тут).

Задумывал ли разработчик, что условие продолжения цикла будет всегда истинным? В теории это возможно, конечно. Но взгляните на цикл повнимательнее. Следующее присваивание в нём выглядит странно:

int toskip = offset - haveskipped;

Какой в этом смысл, если haveskipped всегда равен 0?

С этим циклом явно что-то не то. То ли тут действительно допущена серьёзная ошибка, то ли все эти странности с haveskipped – останки каких-то старых нереализованных идей.

data == null && throw NullReferenceException

Довольно часто ошибки связаны с использованием некорректных операторов в условиях. Нашлось подобное и в компиляторе PHP:

public string ReadStringContents(int maxLength)
{
  if (!CanRead) return null;
  var result = StringBuilderUtilities.Pool.Get();

  if (maxLength >= 0)
  {
    while (maxLength > 0 && !Eof)
    {
      string data = ReadString(maxLength);
      if (data == null && data.Length > 0) break; // EOF or error.
      maxLength -= data.Length;
      result.Append(data);
    }
  }
  ....
}

Предупреждение V3080: Possible null dereference. Consider inspecting 'data'. PhpStream.cs 1382

В цикле производится проверка значения переменной data. Если она равна null и при этом её свойство Length имеет положительное значение, то производится выход из цикла. Очевидно, это невозможно. Более того, обращение к свойству Length переменной, имеющей значение null, приведёт к выбрасыванию исключения. Здесь же обращение подчёркнуто производится именно тогда, когда data = null.

Учитывая комментарий разработчика, я бы переписал условие как-то так:

data == null || data.Length == 0

Тем не менее, не факт, что это действительно корректный вариант обработки – для внесения исправления стоит провести более глубокое исследование кода.

Не то исключение

Бывают и ошибки, которые в принципе не выглядят особенно страшными, но всё же могут доставить проблем. Например, в следующем фрагменте copy-paste нанёс ещё один удар:

public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

Предупреждение V3013: It is odd that the body of 'addGlob' function is fully equivalent to the body of 'addPattern' function (506, line 515). ZipArchive.cs 506

Функция addGlob, очевидно, не поддерживается, поэтому при её вызове выбрасывается исключение, сообщающее о том, что addGlob не поддерживается.

Поверили? А зря! Никакого исключения тут не будет – это ж наш старый знакомый PhpException:

public static class PhpException
{
  ....
  public static void FunctionNotSupported(string/*!*/function)
  {
    Debug.Assert(!string.IsNullOrEmpty(function));

    Throw(PhpError.Warning,
          ErrResources.notsupported_function_called,
          function);
  }
  ....
}

Как мы разбирали ранее, если в метод Throw передаётся значение PhpError.Warning, то исключения не будет. Но всё же возникшая ошибка наверняка будет записана в какой-нибудь лог или обработана как-то ещё.

Вернёмся к исходному фрагменту кода:

public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

addGlob не поддерживается и при вызове соответствующее сообщение будет как-то обработано – будем считать, что оно запишется в лог. addPattern тоже не поддерживается, правда, вот соответствующее сообщение будет всё равно посвящено addGlob.

Очевидно, проблема возникла из-за копирования. Исправить легко – просто в методе addPattern нужно сообщать про addPattern, а не про addGlob:

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addPattern));
  return false;
}

String.Join ни в чём не виноват!

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

public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    if (headers != null)
    {
      var result = new PhpArray(16);

      foreach (var h in headers)
      {
        result[h.Key] = string.Join(", ", h.Value) ?? string.Empty;
      }

      return result;
    }
  }

  return null;
}

Предупреждение V3022: Expression 'string.Join(", ", h.Value)' is always not null. The operator '??' is excessive. Web.cs 932

Использование оператора "??" не имеет здесь никакого смысла, так как метод string.Join никогда не вернёт null. А вот бросить ArgumentNullException – это всегда пожалуйста!

string.Join выбрасывает исключение в том случае, если переданная ссылка на последовательность равна null. Поэтому безопаснее будет записать эту строку как-то так:

result[h.Key] = h.Value != null ? string.Join(", ",h.Value) : string.Empty;

Вообще, я хотел узнать, может ли это Value в принципе быть null. А то, может, тут и вовсе не нужны никакие проверки. Для этого нужно было понять, откуда пришла коллекция headers.

public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    ....
  }

  return null;
}

Значение headers взято из webctx.RequestHeaders, а значение webctx, в свою очередь, берётся из свойства HttpPhpContext объекта ctx. Ну а свойство HttpPhpContext... Что ж, глядите сами:

partial class Context : IEncodingProvider
{
  ....
  public virtual IHttpPhpContext? HttpPhpContext => null;
  ....
}

Это, видимо, некий "на потом". Если взглянете на метод getallheaders ещё разок, то увидите, что он, получается, вообще никогда не отрабатывает и просто возвращает null.

Что, опять поверили? Ну что же с вами делать! Свойство-то виртуальное. Следовательно, чтобы понять, что же фактически может быть им возвращено, надо изучать наследников. Лично я решил на этом остановиться – всё же мне нужно показывать и другие срабатывания.

Маленькое присваивание в большом методе

Чем больше и сложнее метод, тем больше вероятность наличия в нём ошибки. Разработчикам со временем становится сложно ориентироваться в большой куче кода, при этом всегда очень страшно что-то в этом коде менять. Новый код добавляется, старый остаётся прежним, кое-как эта невероятная конструкция вроде работает, ну и слава богу. Нет ничего удивительного в том, что в таком коде оказываются различные странности. К примеру, давайте взглянем на метод inflate_fast:

internal int inflate_fast(....)
{
  ....
  int r;
  ....
  if (c > e)
  {
    // if source crosses,
    c -= e; // wrapped copy
    if (q - r > 0 && e > (q - r))
    {
      do
      {
        s.window[q++] = s.window[r++];
      }
      while (--e != 0);
    }
    else
    {
      Array.Copy(s.window, r, s.window, q, e);
      q += e; r += e; e = 0;                     // <=
    }
    r = 0;                                       // <=
  }
  ....
}

Предупреждение V3008: The 'r' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 621, 619. InfCodes.cs 621

Для начала вот ссылка на полный код. Метод состоит более чем из двух сотен строк кода с кучей вложенных конструкций. Кажется, что разобраться в нём явно было бы непросто.

Срабатывание довольно однозначное – сначала в блоке переменной r присваивается новое значение, а затем оно безусловно перезаписывается нулём. Сложно сказать, что именно здесь не так. То ли обнуление работает как-то не так, то ли конструкция r += e здесь лишняя.

null dereference в логическом выражении

Ранее мы уже рассмотрели случай, когда некорректно построенное логическое выражение приводит к выбрасыванию исключения. Ниже приведён ещё один пример подобного срабатывания:

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null && langVersion.Major > 7 
         || (langVersion.Major == 7 && langVersion.Minor >= 2);
}

Предупреждение V3080: Possible null dereference. Consider inspecting 'langVersion'. AnalysisFacts.cs 20

Код проверяет, что переданный параметр langVersion не равен null. Стало быть, разработчик предполагал, что при вызове действительно может быть передан null. Спасает ли проверка от выбрасывания исключения?

Увы, если переменная langVersion будет равна null, то значение первой части выражения будет равно false. При вычислении же второй части будет выброшено исключение.

Как правило, фрагменты кода для статьи приходится дополнительно форматировать для улучшения читаемости. Этот случай не исключение – выражение, рассмотренное ранее, на самом деле, было записано в одну строку:

Учитывая комментарий, можно легко понять, что здесь либо перепутали приоритеты операторов, либо попросту поставили скобку неправильно. Скорее всего, метод должен выглядеть как-то так:

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null 
         && (   langVersion.Major > 7 
             || langVersion.Major == 7 && langVersion.Minor >= 2);
}

Вот и всё!

На самом деле нет. Анализатор выдал примерно 5 сотен срабатываний на весь проект, и интересных среди них осталось немало. Поэтому я всё же предлагаю вам самостоятельно попробовать PVS-Studio и поглядеть, что новенького он ещё найдёт в этом или других проектах. Кто знает, быть может, вы сможете обнаружить какие-нибудь ошибки, которые будут даже интереснее, чем всё, что я тут наразбирал :). Ну и, конечно же, пишите о них в комментарии. Возможно, найденные вами баги даже попадут в Топ-10 ошибок 2021!

Желаю удачи!