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:
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". Очевидно, разработчик перепутал методы и передал аргументы некорректным образом.
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;
}
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:
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.
Сложно сказать, почему всё написано именно так – конструкция кажется бессмысленной. Скорее всего, здесь всё же допущена ошибка.
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
Во всяком случае, такой вариант выглядит логичнее и не вызывает вопросов.
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.
Едва ли это срабатывание указывает на реальную ошибку – просто кто-то написал лишнюю проверку. Возможно, на то даже были причины – например, разработчику было проще читать именно такой код. Хотя кажется, что использование какой-нибудь переменной или константы для хранения результата сравнения было бы более простым вариантом. Как бы там ни было, конструкция избыточна, и долг статического анализатора – об этом сообщить.
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;
Ещё одно срабатывание, связанное с проверкой на 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. К примеру, использовать "?.". Так или иначе, анализатор всё-таки отработал корректно и даже помог разобраться в этой странной ситуации.
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:
Вроде бы ошибка, верно? Но так ведь совсем не интересно! Предлагаю вам потренировать свои навыки поиска глубинного смысла и в комментариях предложить какое-нибудь объяснение того, зачем всё написано именно таким образом.
Ну а пока идём дальше
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:
.
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);
}
Ну, здесь не обманули – и правда исключение.
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 – останки каких-то старых нереализованных идей.
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
Тем не менее, не факт, что это действительно корректный вариант обработки – для внесения исправления стоит провести более глубокое исследование кода.
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;
}
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.
Что, опять поверили? Ну что же с вами делать! Свойство-то виртуальное. Следовательно, чтобы понять, что же фактически может быть им возвращено, надо изучать наследников. Лично я решил на этом остановиться – всё же мне нужно показывать и другие срабатывания.
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 здесь лишняя.
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);
}
. Ну и, конечно же, пишите о них в комментарии. Возможно, найденные вами баги даже попадут в Топ-10 ошибок 2021!
Желаю удачи!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lipilin. Is PHP compilable?! PVS-Studio searches for errors in PeachPie.
habr.com
Давненько не выходили статьи о проверках 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
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
Вроде бы ошибка, верно? Но так ведь совсем не интересно! Предлагаю вам потренировать свои навыки поиска глубинного смысла и в комментариях предложить какое-нибудь объяснение того, зачем всё написано именно таким образом.
Ну а пока идём дальше
Эти неизменяемые строки… не изменить
Давным-давно, когда вы ещё только просматривали первые срабатывания из этой статьи, мы упоминали неизменяемость экземпляров структуры 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!Желаю удачи!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lipilin. Is PHP compilable?! PVS-Studio searches for errors in PeachPie.
PHP – компилируемый язык?! PVS-Studio ищет ошибки в PeachPie
PHP широко известен как интерпретируемый язык программирования, использующийся в основном для разработки сайтов. Однако немногие знают, что для PHP есть ещё и компилятор под .NET — PeachPie. Но вот...
habr.com