Русский
Русский
English
Статистика
Реклама

Static analysis

ONLYOFFICE Community Server как баги способствуют возникновению проблем с безопасностью

17.12.2020 10:10:32 | Автор: admin
image1.png

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

Введение


ONLYOFFICE Community Server free open-source collaborative system developed to manage documents, projects, customer relationship and email correspondence, all in one place. На своём сайте компания подчёркивает безопасность своих решений такими фразами, как "Run your private office with the ONLYOFFICE" и "Secure office and productivity apps". Но какие-либо инструменты для контроля качества кода, видимо, в процессе разработки не используются.

Всё началось с того, что я просматривал исходники нескольких сетевых приложений в поисках вдохновения для воплощения в жизнь одной своей идеи приложения. В фоне работал анализатор PVS-Studio, и я кидал в общий корпоративный чат смешные ошибки.

Так, несколько примеров ушли в Twitter:

image2.png

Позже представители прокомментировали твит, а ещё позже запостили опровержение проблемы:

image3.png

Скорее всего, это действительно так. Но и очков качеству проекта это не добавляет. Давайте посмотрим, что там ещё нашлось.

"Мастер" проверки входных данных


image4.png

Некоторые подходы разработчиков к проверке входных данных поражают своей оригинальностью.

Предупреждение 1

V3022 Expression 'string.IsNullOrEmpty("password")' is always false. SmtpSettings.cs 104

public void SetCredentials(string userName, string password, string domain){    if (string.IsNullOrEmpty(userName))    {        throw new ArgumentException("Empty user name.", "userName");    }    if (string.IsNullOrEmpty("password"))    {        throw new ArgumentException("Empty password.", "password");    }    CredentialsUserName = userName;    CredentialsUserPassword = password;    CredentialsDomain = domain;}

Как вы заметили, этот фрагмент кода задаёт тон всей статье. Его можно описать фразой "Код смешной, а ситуация страшная". Наверное, надо сильно устать, чтобы перепутать переменную password со строкой "password". Эта ошибка позволяет продолжить выполнение кода с пустым паролем. По словам автора кода, пароль дополнительно проверяется в интерфейсе программы. Но процесс программирования устроен так, что написанные ранее функции часто переиспользуются. Поэтому эта ошибка может проявить себя где угодно в будущем. Всегда помните про важность своевременного выявления ошибок в коде.

Предупреждение 2

V3022 Expression 'String.IsNullOrEmpty("name")' is always false. SendInterceptorSkeleton.cs 36

V3022 Expression 'String.IsNullOrEmpty("sendInterceptor")' is always false. SendInterceptorSkeleton.cs 37

public SendInterceptorSkeleton(  string name,  ....,  Func<NotifyRequest, InterceptorPlace, bool> sendInterceptor){    if (String.IsNullOrEmpty("name"))                           // <=        throw new ArgumentNullException("name");    if (String.IsNullOrEmpty("sendInterceptor"))                // <=        throw new ArgumentNullException("sendInterceptor");    method = sendInterceptor;    Name = name;    PreventPlace = preventPlace;    Lifetime = lifetime;}

Внезапно в коде обнаружился целый ряд подобных ошибок. Если сначала было смешно, то сейчас стоит задуматься о причинах написания такого кода. Может это привычка осталась после перехода с другого языка программирования. В C++ часто приносят ошибки бывшие Python программисты по нашему опыту проверок C++ проектов.

Предупреждение 3

V3022 Expression 'id < 0' is always false. Unsigned type value is always >= 0. UserFolderEngine.cs 173

public MailUserFolderData Update(uint id, string name, uint? parentId = null){    if (id < 0)        throw new ArgumentException("id");    ....}

Переменная id имеет беззнаковый тип uint. Следовательно, здесь проверка бессмысленна. Стоит уделить внимание вызову этой функции. Интересно, что передают в эту функцию. Скорее всего, раньше везде использовался знаковый тип int, а после рефакторинга проверка осталась.

Copy-Paste код


image5.png

Предупреждение 1

V3001 There are identical sub-expressions 'searchFilterData.WithCalendar == WithCalendar' to the left and to the right of the '&&' operator. MailSearchFilterData.cs 131

image6.png

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

image7.png

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

Предупреждение 2

V3030 Recurring check. The '!String.IsNullOrEmpty(user)' condition was already verified in line 173. CommonLinkUtility.cs 176

public static string GetUserProfile(string user, bool absolute){  var queryParams = "";  if (!String.IsNullOrEmpty(user))  {      var guid = Guid.Empty;      if (!String.IsNullOrEmpty(user) && 32 <= user.Length && user[8] == '-')      {        ....}

Строка user проверяется 2 раза подряд одинаковым образом. Пожалуй, этот код можно немного отрефакторить. Хотя кто знает, может, в одном из случаев хотели проверить логическую переменную absolute.

Предупреждение 3

V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless WikiEngine.cs 688

private static LinkType CheckTheLink(string str, out string sLink){    sLink = string.Empty;    if (string.IsNullOrEmpty(str))        return LinkType.None;    if (str[0] == '[')    {        sLink = str.Trim("[]".ToCharArray()).Split('|')[0].Trim();    }    else if (....)    {        sLink = str.Split('|')[0].Trim();    }    sLink = sLink.Split('#')[0].Trim();    // <=    if (string.IsNullOrEmpty(str))         // <=        return LinkType.None;    if (sLink.Contains(":"))    {      ....    }    ....}

Уверен, что ошибку здесь глазами не найти. Анализатор обнаружил бесполезную проверку, которая на деле оказалась скопированным кодом сверху. Вместо переменной str должна проверяться переменная sLink.

Предупреждение 4

V3004 The 'then' statement is equivalent to the 'else' statement. SelectelStorage.cs 461

public override string[] ListFilesRelative(....){    var paths = new List<String>();    var client = GetClient().Result;    if (recursive)    {        paths = client.GetContainerFilesAsync(_private_container, int.MaxValue,            null, MakePath(domain, path)).Result.Select(x => x.Name).ToList();    }    else    {        paths = client.GetContainerFilesAsync(_private_container, int.MaxValue,            null, MakePath(domain, path)).Result.Select(x => x.Name).ToList();    }    ....}

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

Предупреждение 5

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

//TODO: Simplifypublic bool SetUnread(List<int> ids, bool unread, bool allChain = false){    ....    if (!chainedMessages.Any())        return true;    var listIds = allChain        ? chainedMessages.Where(x => x.IsNew == !unread).Select(....).ToList()        : ids;    if (!listIds.Any())        return true;    ....    return true;}

Размер этой функции 135 строк. Даже сами разработчики оставили комментарий, что её надо упростить. С кодом функции определённо надо поработать, т.к. он ещё и возвращает одно значение во всех случаях.

Бесполезные вызовы функций


image8.png

Предупреждение 1

V3010 The return value of function 'Distinct' is required to be utilized. DbTenantService.cs 132

public IEnumerable<Tenant> GetTenants(string login, string passwordHash){  //new password  result = result.Concat(ExecList(q).ConvertAll(ToTenant)).ToList();  result.Distinct();  ....}

Метод Distinct удаляет дубликаты из коллекции. Но в C# большинство подобных методов-расширений не меняют объект, а создают копию. Таким образом, в этом примере список result остаётся всё тем же, что и до вызова метода. Ещё тут можно заметить имена login и passwordHash. Возможно, это очередная проблема с безопасностью.

Предупреждение 2

V3010 The return value of function 'ToString' is required to be utilized. UserPhotoManager.cs 678

private static void ResizeImage(ResizeWorkerItem item){  ....  using (var stream2 = new MemoryStream(data))  {      item.DataStore.Save(fileName, stream2).ToString();      AddToCache(item.UserId, item.Size, fileName);  }  ....}

Метод ToString здесь стандартный. Он возвращает текстовое представление объекта, но возвращаемое значение не используется.

Предупреждение 3

V3010 The return value of function 'Replace' is required to be utilized. TextFileUserImporter.cs 252

private int GetFieldsMapping(....){  ....  if (NameMapping != null && NameMapping.ContainsKey(propertyField))  {      propertyField = NameMapping[propertyField];  }  propertyField.Replace(" ", "");  ....}

Кто-то допустил серьёзную ошибку. Из свойства propertyField необходимо было удалить все пробелы, а этого не происходит, т.к. функция Replace не изменяет исходный объект.

Предупреждение 4

V3038 The '"yy"' argument was passed to 'Replace' method several times. It is possible that other argument should be passed instead. MasterLocalizationResources.cs 38

private static string GetDatepikerDateFormat(string s){    return s        .Replace("yyyy", "yy")        .Replace("yy", "yy")   // <=        .Replace("MMMM", "MM")        .Replace("MMM", "M")        .Replace("MM", "mm")        .Replace("M", "mm")        .Replace("dddd", "DD")        .Replace("ddd", "D")        .Replace("dd", "11")        .Replace("d", "dd")        .Replace("11", "dd")        .Replace("'", "")        ;}

Здесь вызовы функций Replace написаны корректно, но в одном место это сделано со странными одинаковыми аргументами.

Потенциальный NullReferenceException


image9.png

Предупреждение 1

V3022 Expression 'portalUser.BirthDate.ToString()' is always not null. The operator '??' is excessive. LdapUserManager.cs 436

public DateTime? BirthDate { get; set; }private bool NeedUpdateUser(UserInfo portalUser, UserInfo ldapUser){  ....  _log.DebugFormat("NeedUpdateUser by BirthDate -> portal: '{0}', ldap: '{1}'",      portalUser.BirthDate.ToString() ?? "NULL",  // <=      ldapUser.BirthDate.ToString() ?? "NULL");   // <=  needUpdate = true;  ....}

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

Весь список сомнительных мест логгирования выглядит так:

  • V3022 Expression 'ldapUser.BirthDate.ToString()' is always not null. The operator '??' is excessive. LdapUserManager.cs 437
  • V3022 Expression 'portalUser.Sex.ToString()' is always not null. The operator '??' is excessive. LdapUserManager.cs 444
  • V3022 Expression 'ldapUser.Sex.ToString()' is always not null. The operator '??' is excessive. LdapUserManager.cs 445

Предупреждение 2

V3095 The 'r.Attributes["href"]' object was used before it was verified against null. Check lines: 86, 87. HelpCenterStorage.cs 86

public override void Init(string html, string helpLinkBlock, string baseUrl){    ....    foreach (var href in hrefs.Where(r =>    {        var value = r.Attributes["href"].Value;        return r.Attributes["href"] != null               && !string.IsNullOrEmpty(value)               && !value.StartsWith("mailto:")               && !value.StartsWith("http");    }))    {      ....    }    ....}

При парсинге Html или Xml очень опасно обращаться к атрибутам по имени без проверки. Эта ошибка особенно привлекательна тем, что значение атрибута href сначала извлекается, а потом проверяется, присутствует ли он вообще.

Предупреждение 3

V3146 Possible null dereference. The 'listTags.FirstOrDefault' can return default null value. FileMarker.cs 299

public static void RemoveMarkAsNew(....){  ....  var listTags = tagDao.GetNewTags(userID, (Folder)fileEntry, true).ToList();  valueNew = listTags.FirstOrDefault(tag => tag.EntryId.Equals(....)).Count;  ....}

Анализатор обнаружил небезопасное использование результата вызова метода FirstOrDefault. Этот метод возвращают значение по умолчанию, если в списке нет ни одного объекта, удовлетворяющего предикату поиска. Значением по умолчанию для ссылочных типов является пустая ссылка (null). Соответственно, прежде чем использовать полученную ссылку, её следует проверить, а не звать сразу свойство, как здесь.

Предупреждение 4

V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ResCulture.cs 28

public class ResCulture{    public string Title { get; set; }    public string Value { get; set; }    public bool Available { get; set; }    public override bool Equals(object obj)    {        return Title.Equals(((ResCulture) obj).Title);    }    ....}

Ссылки на объекты в C# часто сравнивают со значением null. Следовательно, при перегрузке методов сравнения очень важно предусмотреть такие ситуации и добавить соответствующую проверку в начало функции. А здесь этого не сделали.

Другие баги


image10.png

Предупреждение 1

V3022 Expression is always true. Probably the '&&' operator should be used here. ListItemHistoryDao.cs 140

public virtual int CreateItem(ListItemHistory item){    if (item.EntityType != EntityType.Opportunity ||   // <=        item.EntityType != EntityType.Contact)        throw new ArgumentException();    if (item.EntityType == EntityType.Opportunity &&        (DaoFactory.DealDao.GetByID(item.EntityID) == null ||         DaoFactory.DealMilestoneDao.GetByID(item.StatusID) == null))        throw new ArgumentException();    if (item.EntityType == EntityType.Contact &&        (DaoFactory.ContactDao.GetByID(item.EntityID) == null ||         DaoFactory.ListItemDao.GetByID(item.StatusID) == null))        throw new ArgumentException();    ....}

Вызов метода CreateItem приведёт к возникновению исключения типа ArgumentException. Дело в том, что самое первое условное выражение содержит ошибку. Условие всегда имеет результат true. Ошибка заключается в выборе логического оператора. Надо было использовать оператор &&.

Скорее всего, этот метод ещё ни разу не вызывался, т.к. он виртуальный и до сего момента всегда переопределялся в производных классах.

Чтобы впредь не допускать таких ошибок, рекомендую почитать мою статью и сохранить на неё ссылку: "Логические выражения. Как ошибаются профессионалы". Там приведены и разобраны все ошибочные комбинации из логических операторов.

Предупреждение 2

V3052 The original exception object 'ex' was swallowed. Stack of original exception could be lost. GoogleDriveStorage.cs 267

public DriveFile CopyEntry(string toFolderId, string originEntryId){    var body = FileConstructor(folderId: toFolderId);    try    {        var request = _driveService.Files.Copy(body, originEntryId);        request.Fields = GoogleLoginProvider.FilesFields;        return request.Execute();    }    catch (GoogleApiException ex)    {        if (ex.HttpStatusCode == HttpStatusCode.Forbidden)        {            throw new SecurityException(ex.Error.Message);        }        throw;    }}

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

Вот такое небольшое изменение сделает сгенерированное предупреждение более информативным:

throw new SecurityException(ex.Error.Message, ex);

Хотя вполне возможно, что исключение GoogleApiException скрыли намерено.

Предупреждение 3

V3118 Minutes component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalMinutes' value was intended instead. NotifyClient.cs 281

public static void SendAutoReminderAboutTask(DateTime scheduleDate){    ....    var deadlineReminderDate = deadline.AddMinutes(-alertValue);    if (deadlineReminderDate.Subtract(scheduleDate).Minutes > 1) continue;    ....}

Раньше я считал, что диагностика имеет предупредительный характер. В коде моих проектов она всегда выдавала ложные предупреждения. Здесь же я почти уверен, обнаружилась ошибка. Скорее всего, надо использовать именно свойство TotalMinutes, вместо Minutes.

Предупреждение 4

V3008 The 'key' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 244, 240. Metadata.cs 244

private byte[] GenerateKey(){    var key = new byte[keyLength];    using (var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....))    {        key = deriveBytes.GetBytes(keyLength);    }    return key;}

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

Лучше всего тут было бы перейти на C#8 вместо используемого C#5 и написать более короткий код:

private byte[] GenerateKey(){  using var deriveBytes = new Rfc2898DeriveBytes(Password, Salt, ....);  return deriveBytes.GetBytes(keyLength);}

Не могу сказать, возможен ли апгрейд проекта или нет, но таких мест не мало. Их желательно переписать как-нибудь:

  • V3008 The 'hmacKey' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 256, 252. Metadata.cs 256
  • V3008 The 'hmacHash' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 270, 264. Metadata.cs 270
  • V3008 The 'paths' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 512, 508. RackspaceCloudStorage.cs 512
  • V3008 The 'b' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 265, 264. BookmarkingUserControl.ascx.cs 265
  • V3008 The 'taskIds' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 412, 391. TaskDao.cs 412

На крайний случай, можно не выделять память при объявлении переменной.

Баг в PVS-Studio


image11.png

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

По ходу работы над статьёй у нас нашёлся достаточно глупый баг. Признаём и спешим поделиться.

Код из того же Community Server:

private bool IsPhrase(string searchText){    return searchText.Contains(" ") || searchText.Contains("\r\n") ||                                       searchText.Contains("\n");}

Я должен был привести полное предупреждение анализатора перед кодом, как это делается во всей статье, но в этом то и проблема. Предупреждение выглядит так:

image12.png

Управляющие символы \r и \n не экранируются перед выводом в таблицу.

Заключение


image13.png

Давно мне не попадался столь интересный проект для проверки. Спасибо авторам ONLYOFFCE. Мы хотели с Вами связаться, но обратной связи не последовало.

Мы регулярно пишем подобные статьи. Этому жанру уже более десяти лет. Поэтому разработчикам не стоит принимать критику близко к сердцу. Мы будем рады выдать полную версию отчёта для улучшения проекта или предоставить временную лицензию для проверки проекта. Причём не только проекту CommunityServer, а всем желающим и на ОДИН МЕСЯЦ по промокоду #onlyoffice.

image14.png

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


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. ONLYOFFICE Community Server: how bugs contribute to the emergence of security problems.
Подробнее..

Статический анализ baseline файлы vs diff

25.06.2020 12:16:27 | Автор: admin

В статических анализаторах рано или поздно приходится решать задачу упрощения интеграции в существующие проекты, где поправить все предупреждения на legacy коде невозможно.


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



baseline или так называемый suppress profile


Этот метод имеет несколько названий: baseline файл в Psalm и Android Lint, suppress база (или профиль) в PVS-Studio, code smell baseline в detekt.


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


superlinter --create-baseline baseline.xml ./project

Внутри себя он содержит все предупреждения, которые были выданы на этапе создания.


При запуске статического анализатора с baseline.xml, все предупреждения, которые содержатся в этом файле, будут проигнорированы:


superlinter --baseline baseline.xml ./project

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


Обычно, мы хотим достичь следующего:


  • На новый код выдаются все предупреждения
  • На старый код предупреждения выдаются только если его редактировали
  • (опционально) Переносы файлы не должны выдавать предупреждения на весь файл

То, что мы можем конфигурировать в этом подходе это какие поля предупреждения формируют хеш (или "сигнатуру") срабатывания. Чтобы уйти от проблем смещения строк кода, лучше не добавлять номер строки в эти поля.


Вот примерный список того, что вообще может являться полем сигнатуры:


  • Название или код диагностики
  • Текст предупреждения
  • Название файла
  • Строка исходного кода, на которую сработало предупреждение

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


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


Ещё один менее очевидный признак имя функции или метода, в которой выдавалось предупреждение. Это позволяет снизить количество коллизий, но провоцирует шквал предупреждений на всю функцию, если вы её переименуете.


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


Хорошо подобранный набор признаков увеличивает эффективность baseline подхода.


Коллизии в методе baseline


Допустим, диагностика W104 находит вызовы die в коде.


В проверяемом проекте есть файл foo.php:


function legacy() {  die('test');}

Предположим, используются признаки {имя файла, код диагностики, строка исходного кода}.


Наш анализатор при создании baseline добавляет вызов die('test') в свою базу исключений:


{  "filename": "foo.php",  "diag": "W104",  "line": "die('test');"}

Теперь добавим немного нового кода:


+ function newfunc() {+   die('test');+ }  function legacy() {    die('test');  }

По всем используемым признакам новый вызов die('test') будет распознаваться как игнорируемый код. Совпадение сигнатур для потенциально разных кусков кода мы и называем коллизией.


Одним из решений является добавление дополнительного признака, который позволит различать эти вызовы. К примеру, это может быть поле "имя содержащей функции".


Что делать, если вызов die('test') был добавлен в ту же функцию? Этот вызов может иметь одинаковые соседние строки в обоих случаях, поэтому добавление предыдущей и следующей строки в сигнатуре не поможет.


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


При этом мы теряем некоторую точность: нельзя определить, какая из строк новая, а какая старая. Мы будем ругаться на ту, что будет идти после проигнорированных.


Метод, основанный на diff возможностях VCS


Исходной задачей было выдавать предупреждение только на "новый код". Системы контроля версий как раз могут нам в этом помочь.


Утилита revgrep принимает на stdin поток предупреждений, анализирует git diff и выдаёт на выход только те предупреждения, которые исходят от новых строк.


golangci-lint использует форк revgrep как библиотеку, так что в основе его вычисления diff'а лежат те же алгоритмы.

Если выбран этот путь, придётся искать ответ на следующие вопросы:


  • Как выбрать окно коммитов для вычисления diff?
  • Как будем обрабатывать коммиты, пришедшие из основной ветки (merge/rebase)?

Вдобавок нужно понимать, что иногда мы всё же хотим выдавать предупреждения, выходящие за рамки diff. Пример: вы удалили класс директора по мемам, MemeDirector. Если этот класс упоминался в каких-либо doc-комментариях, желательно, чтобы линтер сообщил об этом.


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


Окно коммитов тоже можно определить по-разному. Скорее всего, вы не захотите проверять только последний коммит, потому что тогда будет возможно пушить сразу два коммита: один с предупреждениями, а второй для обхода CI. Даже если это не будет происходить умышленно, в системе появляется возможность пропустить критический дефект. Также стоит учесть, что предыдущий коммит может быть взят из основной ветки, в этом случае проверять его тоже не следует.


diff режим в NoVerify


NoVerify имеет два режима работы: diff и full diff.


Обычный diff может найти предупреждения по файлам, которые были затронуты изменениями в рамках рассматриваемого диапазона коммитов. Это работает быстро, но хорошего анализа зависимостей изменений не происходит, поэтому новые предупреждения из незатронутых файлов мы найти не можем.


Full diff запускает анализатор дважды: один раз на старом коде, затем на новом коде, а потом фильтрует результаты. Это можно сравнить с генерацией baseline файла на лету с помощью того, что мы можем получить предыдущую версию кода через git. Ожидаемо, этот режим увеличивает время выполнения почти вдвое.


Изначальная схема работы предполагалась такая: на pre-push хуках запускается более быстрый анализ, в режиме обычного diff'а, чтобы люди получали обратную связь как можно быстрее. На CI агентах полный diff. В результате время от времени люди спрашивают, почему на агентах проблемы были найдены, а локально всё чисто. Удобнее иметь одинаковые процессы проверки, чтобы при прохождении pre-push хука была гарантия прохождения на CI фазы линтера.


full diff за один проход


Мы можем делать приближенный к full diff аналог, который не требует двойного анализа кода.


Допустим, в diff попала такая строка:


- class Foo {

Если мы попробуем классифицировать эту строку, то определим её как "Foo class deletion".


Каждая диагностика, которая может как-то зависеть от наличия класса, должна выдавать предупреждение, если имеется факт удаления этого класса.


Аналогично с удалениями переменных (глобальных или локальных) и функций, мы генерируем набор фактов о всех изменениях, которые можем классифицировать.


Переименования не требуют дополнительной обработки. Мы считаем, что символ со старым именем был удалён, а с новым добавлен:


- class MemeManager {+ class SeniorMemeManager {

Основной сложностью будет правильно классифицировать строки изменений и выявить все их зависимости, не замедлив алгоритм до уровня full diff с двойным обходом кодовой базы.


Выводы


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


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


baseline diff
+ легко сделать эффективным + не требует хранить файлов
+ простота реализации и конфигурации + проще отличать новый код от старого
- нужно решать коллизии - сложно правильно приготовить

Могут существовать гибридные подходы: сначала берёшь baseline файл, а потом разрешаешь коллизии и вычисляешь смещения строк кода через git.


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

Подробнее..

Релиз ruleguard v0.3.0

24.01.2021 18:21:50 | Автор: admin

А что, если я скажу вам, что линтеры для Go можно создавать вот таким декларативным способом?


func alwaysTrue(m dsl.Matcher) {    m.Match(`strings.Count($_, $_) >= 0`).Report(`always evaluates to true`)    m.Match(`bytes.Count($_, $_) >= 0`).Report(`always evaluates to true`)}func replaceAll() {    m.Match(`strings.Replace($s, $d, $w, $n)`).        Where(m["n"].Value.Int() <= 0).        Suggest(`strings.ReplaceAll($s, $d, $w)`)}

Год назад я уже рассказывал об утилите ruleguard. Сегодня хотелось бы поделиться тем, что нового появилось за это время.


Основные нововведения:






Небольшое введение


ruleguard это платформа для запуска динамических диагностик. Что-то вроде интерпретатора для скриптов, специализирующихся на статическом анализе.


Вы описываете на DSL свой набор правил (или используете уже готовые наборы) и запускаете их через утилиту ruleguard.




Эти правила интерпретируются во время работы, поэтому нет необходимости повторно собирать анализатор каждый раз, когда вы добавляете новые диагностики. Это особенно важно, если мы рассматриваем интеграцию с golangci-lint. Было бы очень неудобно перекомпилировать golangci-lint при желании использовать свой набор правил.


Если называть наиболее близкие к этой концепции проекты, то в голову приходят CodeQL и Semgrep. Некоторое время назад я проводил сравнение, хотя часть информации из того доклада уже устарела (все проекты получают новые фичи).


Работаю над проектом я в свободное время, когда появляется настроение, поэтому результаты за год могут показаться не такими впечатляющими. Тем не менее проект развивается.


Большая часть нововведений адресует какую-то конкретную проблему, отсюда и формат заголовков.


Терминология, используемая в статье


Поскольку я иногда использую специфичную для проекта терминологию, приведу здесь несколько расшифровок.


EN RU Значение
Rule Правило AST-шаблон, совмещённый с фильтрами и ассоциированными действиями (чаще всего создание предупреждения).
Rules group Группа правил Именованный набор правил. Мы могли бы называть группы "диагностиками", как это делается в других линтерах, но группа не обязана выполнять единственную проверку.
Rule set Набор правил Совокупность групп правил.
Rule bundle Бандл (извините) Набор правил, оформленный как Go модуль, доступный для импортирования в другие наборы правил.
Module Модуль Модули Go; каждый бандл модуль, но сами модули к бандлам не имеют никакого отношения.

Если по мере прочтения статьи вы нашли совершенно непонятный для вас термин, стоит сообщить об этом, возможно он будет добавлен в эту таблицу.




Проблема: переиспользование наборов правил


Раньше всё было относительно просто: есть файл с правилами, утилита принимает его на вход и применяет его к проверяемой кодовой базе.


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


Затем появился хороший набор правил, написанный Damian Gryski. Единственный способ его использовать на своих проектах это копировать в свой репозиторий.


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


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


  • Установка бандлов через go get
  • Версионирование с помощью Go модулей: удобно делать релизы и закреплять версию
  • Культура оформления правил в модули упрощает тестирование

Всё это возможно благодаря тому, что ruleguard файлы, в которых пишутся правила это обычный Go код (по этой же причине мы имеем нормальный autocomplete и поддержку редакторов).


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


package gorulesimport (    "github.com/quasilyte/go-ruleguard/dsl"    damianrules "github.com/dgryski/semgrep-go")func init() {    // Импорт всех правил, без префикса.    dsl.ImportRules("", damianrules.Bundle)}func emptyStringTest(m dsl.Matcher) {    m.Match(`len($s) == 0`).        Where(m["s"].Type.Is("string")).        Report(`maybe use $s == "" instead?`)    m.Match(`len($s) != 0`).        Where(m["s"].Type.Is("string")).        Report(`maybe use $s != "" instead?`)}

Если требуется выключить некоторые импортируемые правила, делается это через командную строку параметром -disable.


Проблема: недостаточная выразительность DSL


dsl.Matcher предоставляет несколько фильтров, которые часто нужны в типичных для ruleguard правилах.


Но бывают моменты, когда требуется создать довольно сложное условие или фильтр, имеющий промежуточные результаты. В этой ситуации можно использовать новый метод Filter(), который принимает Go функцию-предикат в качестве аргумента. Эта функция будет вызываться во время применения фильтра.


package gorulesimport (    "github.com/quasilyte/go-ruleguard/dsl"    "github.com/quasilyte/go-ruleguard/dsl/types")// implementsStringer является пользовательским фильтром.// Этот фильтр проверяет, реализуют ли T или *T интерфейс `fmt.Stringer`.func implementsStringer(ctx *dsl.VarFilterContext) bool {    stringer := ctx.GetInterface(`fmt.Stringer`)    return types.Implements(ctx.Type, stringer) ||        types.Implements(types.NewPointer(ctx.Type), stringer)}func sprintStringer(m dsl.Matcher) {    // Если бы мы использовали m["x"].Type.Implements(`fmt.Stringer`), тогда    // мы бы не получили все желаемые результаты: если тип $x реализует    // fmt.Stringer как *T, то значения типа T не будут считаться реализациями.    // Наш кастомный фильтр примеряет обе версии: с указателем и без укатателя.    m.Match(`fmt.Sprint($x)`).        Where(m["x"].Filter(implementsStringer) && m["x"].Addressable).        Report(`can use $x.String() directly`)}

Запускать эти правила будем на следующем файле:


package mainimport "fmt"func main() {    fooPtr := &Foo{}    foo := Foo{}    println(fmt.Sprint(foo))    println(fmt.Sprint(fooPtr))    println(fmt.Sprint(0))    // Не fmt.Stringer    println(fmt.Sprint(&foo)) // Отбрасывается условием addressable}type Foo struct{}func (*Foo) String() string { return "Foo" }

Результат запуска:


$ ruleguard -rules rules.go main.gomain.go:9:10: can use foo.String() directlymain.go:10:10: can use fooPtr.String() directly

Флаг -debug-filter позволяет посмотреть, во что скомпилировался выбранный фильтр:



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


Проблема: правила сложно отлаживать


Поскольку в Where() может использоваться довольно сложное выражение, не всегда понятно, почему правило не срабатывает на анализируемых фрагментах кода.


На помощь приходит новый флаг debug-group, включающий детальную информацию о неуспешно выполнившихся фильтрах для выбранной группы правил.


Допустим, вы описали следующее правило:


func offBy1(m dsl.Matcher) {    m.Match(`$s[len($s)]`).        Where(m["s"].Type.Is(`[]$elem`) && m["s"].Pure).        Report(`index expr always panics; maybe you wanted $s[len($s)-1]?`)}

И запустили его на следующем файле:


func lastByte(s string) byte {    return s[len(s)]}func f() byte {    return randString()[len(randString())]}

И не получили ни одного предупреждения Давайте попробуем включить отладочную печать.


$ ruleguard -rules rules.go -debug-group offBy1 test.gotest.go:6: [rules.go:6] rejected by m["s"].Type.Is(`[]$elem`)  $s string: stest.go:10: [rules.go:6] rejected by m["s"].Pure  $s []byte: randBytes()

Мы видим конкретное выражение из Where(), которое не дало сработать правилу. Мы также видим все захваченные Go выражения в именованных частях AST шаблона (в данном случае это $s), а также их тип.


В первом случае условие типа []$elem требует произвольного слайса, а в коде строка. Во втором случае правило не срабатывает из-за вызова функции (нарушается условие pure).


Скорее всего, мы не хотим убирать условие на чистоту выражений, а вот добавить тип string в диагностику можно:


- Where(m["s"].Type.Is(`[]$elem`) && m["s"].Pure).+ Where((m["s"].Type.Is(`[]$elem`) || m["s"].Type.Is(`string`)) && m["s"].Pure).

Повторный запуск с обновлённой версией найдёт ошибку в индексировании строки:


test.go:6:9: offBy1: index expr always panics; maybe you wanted s[len(s)-1]?

Проблема: трудности изучения DSL


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


Мне нравится подход Go by Example. В нём введение производится через набор примеров с пояснениями, от простого к более продвинутому. Это полезно как начинающим, так и продолжающим.


Ruleguard by Example написан в таком же стиле. Он позволяет достаточно быстро получить все необходимые знания в наглядной форме.



Как начать использовать ruleguard?


Внимание! Лучше всего ruleguard работает с проектами, которые используют Go модули.

Лучше всего дождаться момента, когда в golangci-lint появится новая версия.


Однако, если вы не используете golangci-lint или хотите попробовать уже сегодня, то можно скачать бинарник ruleguard со страницы релиза {linux/amd64, linux/arm64, darwin/amd64, windows/amd64}.


Вам также понадобится набор правил. Здесь есть как минимум два варианта: использовать минималистичный набор github.com/quasilyte/go-ruleguard/rules или более обширный github.com/dgryski/semgrep-go. Вы также можете импортировать оба этих бандла или не импортировать ничего и использовать лишь свои наработки.


Допустим, вы выбрали github.com/quasilyte/go-ruleguard/rules, тогда:


  1. Скачиваем ruleguard для своей платформы (или собираем из исходников)
  2. Выполняем go get -v github.com/quasilyte/go-ruleguard/dsl внутри модуля вашего проекта
  3. Выполняем go get -v github.com/quasilyte/go-ruleguard/rules внутри модуля вашего проекта
  4. Создаём свой файл правил rules.go, импортируем там установленный бандл
  5. Запускаем ruleguard с параметром -rules rules.go на вашем проекте

$ ruleguard -rules rules.go ./...

Если у вас возникают проблемы с запуском или установкой ruleguard, сообщите об этом.


Создаём свой бандл


Есть только два требования:


  1. Бандл должен быть отдельным Go модулем
  2. Пакет должен определять экспортируемую переменную Bundle

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


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


package gorulesimport "github.com/quasilyte/go-ruleguard/dsl"// Bundle содержит метаданные о наборе правил.var Bundle = dsl.Bundle{}func boolComparison(m dsl.Matcher) {    m.Match(`$x == true`,        `$x != true`,        `$x == false`,        `$x != false`).        Report(`omit bool literal in expression`)}

В качестве примера, можно посмотреть на репозиторий ruleguard-rules-test.


Тестируем свой бандл


Тестирование основано на фреймворке go/analysis и вспомогательном пакете analysistest.


Рядом с модулем создаётся директория testdata, куда складываются Go файлы, на которых будут запускаться ваши диагностики.


Для запуска тестов нужно написать некоторый шаблонный код:


// file rules_test.gopackage gorules_testimport (    "testing"    "github.com/quasilyte/go-ruleguard/analyzer"    "golang.org/x/tools/go/analysis/analysistest")func TestRules(t *testing.T) {    // Если у вас несколько файлов с правилами, то вместо "rules.go"    // нужно указать имена всех файлов через запятую, например: "style.go,perf.go".    if err := analyzer.Analyzer.Flags.Set("rules", "rules.go"); err != nil {        t.Fatalf("set rules flag: %v", err)    }    analysistest.Run(t, analysistest.TestData(), analyzer.Analyzer, "./...")}

Структура бандла будет выглядеть примерно так:


mybundle/  go.mod        -- файл, создаваемый "go mod init"  rules.go      -- здесь ваши правила (можно назвать файл иначе)  rules_test.go -- запускатель тестов  testdata/     -- файлы, на которых будем запускать анализ    target1.go    target2.go    ...

Тестовые файлы будут содержать магические комментарии:


// file testdata/target1.gopackage testfunc f(cond bool) {    if cond == true { // want `omit bool literal in expression`    }}

После want идёт регулярное выражение, которое должно матчить выдаваемое предупреждение. Могу рекомендовать использовать \Q в начале, чтобы не приходилось ничего экранировать.


Тест запускается обычным go test из директории бандла.


Ссылки и дополнительные материалы



Подробнее..

Код игры Command amp Conquer баги из 90-х. Том второй

13.07.2020 10:06:40 | Автор: admin
image1.png

Американская компания Electronic Arts Inc (EA) выложила в открытый доступ исходный код игр Command & Conquer: Tiberian Dawn и Command & Conquer: Red Alert. В исходном коде было обнаружено несколько десятков ошибок с помощью анализатора PVS-Studio, поэтому встречайте продолжение описания найденных дефектов.

Введение


Command & Conquer серия компьютерных игр в жанре стратегии в реальном времени. Первая игра серии была выпущена в 1995 году. Исходный код игр опубликовали вместе с выпуском коллекции Command & Conquer Remastered.

Для поиска ошибок в коде использовался анализатор PVS-Studio. Это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java.

Ссылка на первый обзор ошибок: "Игра Command & Conquer: баги из 90-х. Том первый".

Ошибки в условиях


V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 3072. STARTUP.CPP 1136

void Read_Setup_Options( RawFileClass *config_file ){  ....  ScreenHeight = ini.Get_Bool("Options", "Resolution", false) ? 3072 : 3072;  ....}

Оказывается, на некоторые настройки пользователи не могли повлиять. Точнее они что-то делали, но из-за того, что тернарный оператор всегда возвращает одно значение, по факту ничего не менялось.

V590 Consider inspecting the 'i < 8 && i < 4' expression. The expression is excessive or contains a misprint. DLLInterface.cpp 2238

// Maximum number of multi players possible.#define MAX_PLAYERS 8 // max # of players we can havefor (int i = 0; i < MAX_PLAYERS && i < 4; i++) {  if (GlyphxPlayerIDs[i] == player_id) {    MultiplayerStartPositions[i] = XY_Cell(x, y);  }}

Из-за неправильного цикла не задаётся позиция для всех игроков. С одной стороны, мы видим константу MAX_PLAYERS 8 и предполагаем, что это максимальное количество игроков. С другой мы видим условие i < 4 и оператор &&. Таким образом, цикл никогда не делает 8 итераций. Скорее всего, на начальном этапе разработки программист не использовал константы, а когда начал забыл удалить старые числа из кода.

V648 Priority of the '&&' operation is higher than that of the '||' operation. INFANTRY.CPP 1003

void InfantryClass::Assign_Target(TARGET target){  ....  if (building && building->Class->IsCaptureable &&    (GameToPlay != GAME_NORMAL || *building != STRUCT_EYE && Scenario < 13)) {    Assign_Destination(target);  }  ....}

Сделать код неочевидным (и, скорее всего, ошибочным) можно просто не указав приоритет операций для операторов || и &&. Здесь совсем непонятно, это ошибка или нет. Но учитывая общее качество кода этих проектов, предположим, что здесь и ещё в нескольких местах допущены ошибки с приоритетом операций:

  • V648 Priority of the '&&' operation is higher than that of the '||' operation. TEAM.CPP 456
  • V648 Priority of the '&&' operation is higher than that of the '||' operation. DISPLAY.CPP 1160
  • V648 Priority of the '&&' operation is higher than that of the '||' operation. DISPLAY.CPP 1571
  • V648 Priority of the '&&' operation is higher than that of the '||' operation. HOUSE.CPP 2594
  • V648 Priority of the '&&' operation is higher than that of the '||' operation. INIT.CPP 2541

V617 Consider inspecting the condition. The '((1L << STRUCT_CHRONOSPHERE))' argument of the '|' bitwise operation contains a non-zero value. HOUSE.CPP 5089

typedef enum StructType : char {  STRUCT_NONE=-1,  STRUCT_ADVANCED_TECH,  STRUCT_IRON_CURTAIN,  STRUCT_WEAP,  STRUCT_CHRONOSPHERE, // 3  ....}#define  STRUCTF_CHRONOSPHERE (1L << STRUCT_CHRONOSPHERE)UrgencyType HouseClass::Check_Build_Power(void) const{  ....  if (State == STATE_THREATENED || State == STATE_ATTACKED) {    if (BScan | (STRUCTF_CHRONOSPHERE)) {  // <=      urgency = URGENCY_HIGH;    }  }  ....}

Чтобы проверить, выставлены ли определённые биты в переменной, следует использовать оператор &, а не |. Из-за опечатки в этом фрагменте кода получилось всегда истинное условие.

V768 The enumeration constant 'WWKEY_RLS_BIT' is used as a variable of a Boolean-type. KEYBOARD.CPP 286

typedef enum {  WWKEY_SHIFT_BIT = 0x100,  WWKEY_CTRL_BIT  = 0x200,  WWKEY_ALT_BIT   = 0x400,  WWKEY_RLS_BIT   = 0x800,  WWKEY_VK_BIT    = 0x1000,  WWKEY_DBL_BIT   = 0x2000,  WWKEY_BTN_BIT   = 0x8000,} WWKey_Type;int WWKeyboardClass::To_ASCII(int key){  if ( key && WWKEY_RLS_BIT)    return(KN_NONE);  return(key);}

Я думаю, в параметре key хотели проверить определённый бит, заданный маской WWKEY_RLS_BIT, но сделали опечатку. Следовало использовать побитовый оператор &, а не &&, чтобы проверить код клавиши.

Подозрительное форматирование


V523 The 'then' statement is equivalent to the 'else' statement. RADAR.CPP 1827

void RadarClass::Player_Names(bool on){  IsPlayerNames = on;  IsToRedraw = true;  if (on) {    Flag_To_Redraw(true);//    Flag_To_Redraw(false);  } else {    Flag_To_Redraw(true);   // force drawing of the plate  }}

Когда-то разработчик комментировал код для отладки. С тех пор в коде остался условный оператор с одинаковыми операторами в разных ветках.

Точно таких же мест нашлось ещё два:

  • V523 The 'then' statement is equivalent to the 'else' statement. CELL.CPP 1792
  • V523 The 'then' statement is equivalent to the 'else' statement. RADAR.CPP 2274

V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. NETDLG.CPP 1506

static int Net_Join_Dialog(void){  ....  /*...............................................................  F4/SEND/'M' = edit a message  ...............................................................*/  if (Messages.Get_Edit_Buf()==NULL) {    ....  } else  /*...............................................................  If we're already editing a message and the user clicks on  'Send', translate our input to a Return so Messages.Input() will  work properly.  ...............................................................*/  if (input==(BUTTON_SEND | KN_BUTTON)) {    input = KN_RETURN;  }  ....}

Из-за большого комментария разработчик не увидел выше недописанный условный оператор. Оставшееся ключевое слово else образует с условием ниже конструкцию else if, что, скорее всего, является изменением изначальной логики.

V519 The 'ScoresPresent' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 539, 541. INIT.CPP 541

bool Init_Game(int , char *[]){  ....  ScoresPresent = false;//if (CCFileClass("SCORES.MIX").Is_Available()) {    ScoresPresent = true;    if (!ScoreMix) {      ScoreMix = new MixFileClass("SCORES.MIX");      ThemeClass::Scan();    }//}

Ещё один потенциальный дефект из-за незаконченного рефакторинга. Теперь непонятно, переменная ScoresPresent должна иметь значение true, или всё-таки false.

Ошибки освобождения памяти


V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] poke_data;'. CCDDE.CPP 410

BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type){  ....  char *poke_data = new char [length + 2*sizeof(int)]; // <=  ....  if(DDE_Class->Poke_Server( .... ) == FALSE) {    CCDebugString("C&C95 - POKE failed!\n");    DDE_Class->Close_Poke_Connection();    delete poke_data;                                  // <=    return (FALSE);  }  DDE_Class->Close_Poke_Connection();  delete poke_data;                                    // <=  return (TRUE);}

Анализатор обнаружил ошибку, связанную с тем, что память может выделяется и освобождаться несовместимыми между собой способами. Для освобождения памяти, выделенной под массив, следовало использовать оператор delete[], а не delete.

Таких мест нашлось несколько, и все они понемногу вредят работающему приложению (игре):

  • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] poke_data;'. CCDDE.CPP 416
  • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] temp_buffer;'. INIT.CPP 1302
  • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] progresspalette;'. MAPSEL.CPP 795
  • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] grey2palette;'. MAPSEL.CPP 796
  • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] poke_data;'. CCDDE.CPP 422
  • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] temp_buffer;'. INIT.CPP 1139

V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. ENDING.CPP 254

void GDI_Ending(void){  ....  void * localpal = Load_Alloc_Data(CCFileClass("SATSEL.PAL"));  ....  delete [] localpal;  ....}

Операторы delete и delete[] разделены неслучайно. Они выполняют разную работу по очистке памяти. А при использовании нетипизированного указателя компилятор не знает, на какой тип данных ведёт указатель. В стандарте языка C++ поведение компилятора неопределённо.

Такого рода тоже нашёлся целый ряд предупреждений анализатора:

  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. HEAP.CPP 284
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. INIT.CPP 728
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 134
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 391
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MSGBOX.CPP 423
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. SOUNDDLG.CPP 407
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFFER.CPP 126
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFF.CPP 162
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFF.CPP 212
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BFIOFILE.CPP 330
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. EVENT.CPP 934
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. HEAP.CPP 318
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. INIT.CPP 3851
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 130
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 430
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 447
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 481
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MSGBOX.CPP 461
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. QUEUE.CPP 2982
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. QUEUE.CPP 3167
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. SOUNDDLG.CPP 406

V773 The function was exited without releasing the 'progresspalette' pointer. A memory leak is possible. MAPSEL.CPP 258

void Map_Selection(void){  ....  unsigned char *grey2palette    = new unsigned char[768];  unsigned char *progresspalette = new unsigned char[768];  ....  scenario = Scenario + ((house == HOUSE_GOOD) ? 0 : 14);  if (house == HOUSE_GOOD) {    lastscenario = (Scenario == 14);    if (Scenario == 15) return;  } else {    lastscenario = (Scenario == 12);    if (Scenario == 13) return;  }  ....}

"Если вообще не освобождать память, то точно не ошибусь в выборе оператора!" возможно, подумал программист.

image2.png

Но тогда происходит утечка памяти, что тоже является ошибкой. Где-то в конце функции выполняется освобождение памяти, но до этого много мест, где происходит условный выход из функции, и память по указателям grey2palette и progresspalett не освобождается.

Разное


V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 806

struct CommHdr {  unsigned short MagicNumber;  unsigned char Code;  unsigned long PacketID;} *hdr;void CommBufferClass::Mono_Debug_Print(int refresh){  ....  hdr = (CommHdr *)SendQueue[i].Buffer;  hdr->MagicNumber = hdr->MagicNumber;  hdr->Code = hdr->Code;  ....}

Два поля структуры CommHdr инициализируются собственными значениями. По-моему, бессмысленная операция, но выполняется она много раз:

  • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 807
  • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 931
  • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 932
  • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 987
  • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 988
  • V570 The 'obj' variable is assigned to itself. MAP.CPP 1132
  • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 910
  • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 911
  • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 1040
  • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 1041
  • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 1104
  • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 1105
  • V570 The 'obj' variable is assigned to itself. MAP.CPP 1279

V591 Non-void function should return a value. HEAP.H 123

int FixedHeapClass::Free(void * pointer);template<class T>class TFixedHeapClass : public FixedHeapClass{  ....  virtual int Free(T * pointer) {FixedHeapClass::Free(pointer);};};

В функции Free класса TFixedHeapClass нет оператора return. Самое интересное, что у вызываемой функции FixedHeapClass::Free возвращаемое значение тоже типа int. Скорее всего, программист просто забыл написать оператор return и теперь функция возвращает непонятное значение.

V672 There is probably no need in creating the new 'damage' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 1219, 1278. BUILDING.CPP 1278

ResultType BuildingClass::Take_Damage(int & damage, ....){  ....  if (tech && tech->IsActive && ....) {    int damage = 500;    tech->Take_Damage(damage, 0, WARHEAD_AP, source, forced);  }  ....}

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

Ещё одно такое место:

  • V672 There is probably no need in creating the new 'damage' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 4031, 4068. TECHNO.CPP 4068

V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Occupy_List' in derived class 'BulletClass' and base class 'ObjectClass'. BULLET.H 90

class ObjectClass : public AbstractClass{  ....  virtual short const * Occupy_List(bool placement=false) const; // <=  virtual short const * Overlap_List(void) const;  ....};class BulletClass : public ObjectClass,                    public FlyClass,                    public FuseClass{  ....  virtual short const * Occupy_List(void) const;                 // <=  virtual short const * Overlap_List(void) const {return Occupy_List();};  ....};

Анализатор обнаружил потенциальную ошибку при переопределении виртуальной функции Occupy_List. Это может приводить к вызову не тех функций в рантайме.

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

  • V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'Ok_To_Move' in derived class 'TurretClass' and base class 'DriveClass'. TURRET.H 76
  • V762 It is possible a virtual function was overridden incorrectly. See fourth argument of function 'Help_Text' in derived class 'HelpClass' and base class 'DisplayClass'. HELP.H 55
  • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Draw_It' in derived class 'MapEditClass' and base class 'HelpClass'. MAPEDIT.H 187
  • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Occupy_List' in derived class 'AnimClass' and base class 'ObjectClass'. ANIM.H 80
  • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Overlap_List' in derived class 'BulletClass' and base class 'ObjectClass'. BULLET.H 102
  • V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'Remap_Table' in derived class 'BuildingClass' and base class 'TechnoClass'. BUILDING.H 281
  • V762 It is possible a virtual function was overridden incorrectly. See fourth argument of function 'Help_Text' in derived class 'HelpClass' and base class 'DisplayClass'. HELP.H 58
  • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Overlap_List' in derived class 'AnimClass' and base class 'ObjectClass'. ANIM.H 90

V763 Parameter 'coord' is always rewritten in function body before being used. DISPLAY.CPP 4031

void DisplayClass::Set_Tactical_Position(COORDINATE coord){  int xx = 0;  int yy = 0;  Confine_Rect(&xx, &yy, TacLeptonWidth, TacLeptonHeight,    Cell_To_Lepton(MapCellWidth) + GlyphXClientSidebarWidthInLeptons,    Cell_To_Lepton(MapCellHeight));  coord = XY_Coord(xx + Cell_To_Lepton(MapCellX), yy + Cell_To_Lepton(....));  if (ScenarioInit) {    TacticalCoord = coord;  }  DesiredTacticalCoord = coord;  IsToRedraw = true;  Flag_To_Redraw(false);}

Параметр coord сразу перезаписывается в теле функции. Старое значение не использовалось. Это очень подозрительно, когда у функции есть аргументы, а она от них не зависит. А тут ещё координаты какие-то передают.

И это место стоит проверить:

  • V763 Parameter 'coord' is always rewritten in function body before being used. DISPLAY.CPP 4251

V507 Pointer to local array 'localpalette' is stored outside the scope of this array. Such a pointer will become invalid. MAPSEL.CPP 757

extern "C" unsigned char *InterpolationPalette;void Map_Selection(void){  unsigned char localpalette[768];  ....  InterpolationPalette = localpalette;  ....}

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

В указатель InterpolationPalette сохраняется локальный массив localpalette, который станет невалидным после выхода из функции.

Ещё парочка опасных мест:

  • V507 Pointer to local array 'localpalette' is stored outside the scope of this array. Such a pointer will become invalid. MAPSEL.CPP 769
  • V507 Pointer to local array 'buffer' is stored outside the scope of this array. Such a pointer will become invalid. WINDOWS.CPP 458

Заключение


Как я уже писал в первом отчёте, будем надеяться, что новые проекты Electronic Arts более качественные. Вообще, разработчики игр активно приобретают PVS-Studio. Сейчас бюджеты игр достаточно велики, поэтому лишние расходы на исправление багов в продакшене никому не нужны. А исправление ошибки на раннем этапе написания кода практически не отнимает время и другие ресурсы.

Приглашаем на наш сайт скачать и попробовать PVS-Studio на всех проектах.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. The Code of the Command & Conquer Game: Bugs from the 90's. Volume two.
Подробнее..

Новые возможности PVS-Studio по оповещению разработчиков о найденных ошибках

18.05.2021 16:06:28 | Автор: admin

В поддержку PVS-Studio часто поступают предложения от пользователей по улучшению продукта. Многие из них мы с радостью берёмся реализовывать. Одно из последних таких предложений было связано с доработкой утилиты автоматического оповещения разработчиков (Blame Notifier). Нас попросили научить ее извлекать дату/ревизию кода, на который анализатор выдал сообщение, с помощью blame информации из системы контроля версий. Такая доработка позволила расширить возможности утилиты, о которых мы и поговорим в этой статье.

С чего все началось

Один из наших клиентов написал в поддержку о том, что у него есть необходимость различать предупреждения анализатора на свежие и найденные ранее. В переписке выяснилось, что ему нужно было сосредоточиться на предупреждениях на новый или только-только изменённый код. Мы поспешили сообщить, что это возможно осуществить при помощи SonarQube в связке с нашим плагином. Ответ клиента не заставил себя долго ждать. Он заметил, что пользоваться сторонним и достаточно массивным инструментом, таким как SonarQube, для этих целей нецелесообразно.

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

И тут нас озарило. Ранее мы думали, что для более умного и информативного оповещения нужно создавать собственную базу данных. Отталкиваясь от неё, мы могли бы отслеживать жизненный цикл каждого предупреждения и, соответственно, определять, какие из них являются новыми. Но после замечания клиента мы пришли к выводу, что система контроля версий и есть та самая база данных, из которой можно получать всю необходимую актуальную информацию. Ведь через привязку к номерам ревизий коммитов мы можем вычислить и их привязку ко времени модификации соответствующих строк кода. Более того, мы уже достаём эту информацию через blame осталось лишь воспользоваться ей.

Мы предложили клиенту следующий вариант:

  • результирующий отчёт анализатора вы будете получать со всеми предупреждениями;

  • Blame Notifier можно настроить так, чтобы в HTML-отчёт попадали предупреждения только на свежеизменённый код. Помимо этого, предупреждения будут содержать дату и номер ревизии.

На этом мы и сошлись!

Об утилите

Предназначение Blame Notifier автоматизировать процесс оповещения разработчиков, на код которых выдал предупреждение PVS-Studio, например, после ночных сборок. Утилита позволяет формировать HTML-отчёт как для конкретного разработчика только с его предупреждениями, так и для супер-пользователя, который получает полный отчёт со всеми предупреждениями. Полный отчёт по умолчанию представляет собой сгруппированные предупреждения по разработчикам, а те, в свою очередь, отсортированы в алфавитном порядке. Такая функциональность крайне полезна, так как сразу же будет сигнализировать о новых срабатываниях анализатора всем заинтересованным лицам. Как можно догадаться из названия данной утилиты, она работает на основе blame информации, получаемой для проверяемых анализатором файлов из системы контроля версий пользователя.

Кто незнаком с данной утилитой, подробнее можно познакомиться с ней в этой статье.

Что же нового

Из blame информации, помимо имени разработчика, теперь извлекается дата и ревизия последнего изменения кода, на который ругается PVS-Studio. Дополнительная извлекаемая информация позволила нам добавить в утилиту новые опции:

  • --sortByDate (-S) позволяет формировать HTML-отчёт с отсортированными предупреждениями по дате изменения исходного кода, из-за которого было выдано предупреждение анализатора. Предупреждения для конкретной даты группируются, в свою очередь, по разработчикам.

  • --days (-d) в HTML-отчёт попадают предупреждения на код, дата изменения которого меньше N дней от даты текущего запуска утилиты.

Примечание. Извлечение даты/ревизии поддержано для следующих систем контроля версий: SVN, Git и Mercurial.

Формат HTML-отчёта утилиты по умолчанию выглядит следующим образом:

Новый HTML-отчёт, отсортированный по дате:

Как это можно применить

Новые опции позволяют имитировать поведение SonarQube, когда загружается отчёт анализатора при помощи 'sonar-scanner'. Об этом поподробней.

Примечание. Если вы уже используете PVS-Studio в связке с SonarQube, то описанное ниже применение будет для вас неактуальным, так как соответствующая обработка новых предупреждений в SonarQube уже встроена. А вот если вы используете PVS-Studio отдельно и нет возможности\желания воспользоваться SonarQube, то это может вас заинтересовать.

Начнем с того, что у SonarQube используется подход к качеству кода, называемый 'Clean as You Code'. Его суть в том, что разработчики должны уделять повышенное внимание надежности и безопасности нового кода, который был только-только добавлен или изменен. Старый код, который уже годами доказывает свою работоспособность в 'production', сместить на второй план и сосредоточиться на том, что происходит "сегодня", тем самым предотвращая появление новых проблем. А к давно существующим проблемам периодически возвращаться и исправлять. С этим подходом подробней можно ознакомиться в блоге разработчиков SonarQube.

Данный подход реализован следующим образом. На главной странице проекта есть выделенная область с новыми проблемами за настраиваемый промежуток времени. Для новых проблем настраивается 'Quality Gate', так называемый индикатор соответствия нового кода заданным пороговым метрикам. Например:

  • количество новых багов;

  • количество новых уязвимостей;

  • коэффициент технического долга;

  • покрытие нового кода тестами;

  • ... и другие.

Например, каждое утро будет загружаться свежий отчёт анализатора, и если пороговое значение какой-либо метрики превышено, то 'Quality Gate' будет сигнализировать вам об этом.

Визуальное представление подхода 'Clean as You Code' интуитивно понятное. Вот пример, как это выглядит в SonarQube 7.9.4:

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

  • разбивает проблемы в коде на существующие и свежие;

  • предоставляет различные метрики и графики;

  • позволяет фильтровать проблемы по критериям;

  • позволяет смотреть предупреждения, найденные инструментами контроля качества кода, непосредственно в проверяемом коде прямо из web-браузера;

  • ... и многое другое.

Если SonarQube уже используется, то, чтобы интегрировать с ним результаты работы анализатора PVS-Studio, вам нужно ознакомиться со статьей.

А вот что делать, если вы ещё не используете SonarQube?

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

  • запуске сервера SonarQube;

  • настройке 'Quality Profiles';

  • настройке 'Quality Gates';

  • использовании 'sonar-scanner';

  • ... и так далее.

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

И здесь-то как раз на помощь приходит наша утилита Blame Notifier. Она и раньше уже могла заместить часть возможностей SonarQube в области уведомления разработчиков. С новыми же возможностями утилиты теперь можно имитировать облегченную версию подхода 'Clean as You Code', где основной метрикой качества кода будет появление новых предупреждений анализатора. В таком режиме рассылка будет содержать предупреждения на код, дата изменения которого меньше N дней от даты текущего запуска утилиты.

Повторим поведение SonarQube. Для этого укажем 10 дней для опции '-d' и отсортируем предупреждения (-S) по дате изменения кода, вызвавшего предупреждение. В таком случае HTML-отчёт будет следующим:

Как видно, отчёт содержит 11 новых предупреждений за последние 10 дней. Выглядит практически так же, как и в SonarQube, но получить такую отчётность в разы легче.

У такого "облегченного" подхода, конечно, есть и свои недостатки:

  • качество нового кода оценивается по одной метрике;

  • срабатывания на код, дата изменения которого за пределами рассматриваемого периода, не будут включены в HTML-отчёт. Для этого необходимо получить полный отчёт за все время при помощи дополнительного запуска 'blame-notifier' без ограничивающих опций.

  • ну и, конечно, отсутствует возможность навигации по проверяемому коду непосредственно с помощью web-браузера

Стоит также упомянуть, что использование утилиты Blame Notifier и PVS-Studio плагина для SonarQube доступно только в Enterprise лицензии PVS-Studio.

Заключение

Внедрение технологии статического анализа в разработку подразумевает не только запуск анализатора и получение отчёта. Обычно рука об руку со статическим анализатором идут вспомогательные утилиты/плагины, использование которых позволяет легко и удобно поддерживать качество вашего кода.

Например, PVS-Studio имеет множество плагинов (для Visual Studio, SonarQube, Jenkins, Gradle/Maven/IntelliJ IDEA), утилиту для более удобной конвертации отчётов PlogConverter, утилиту оповещения разработчиков blame-notifier. Команда PVS-Studio, опираясь на свой опыт и на обратную связь пользователей, постоянно совершенствует свой продукт. Чтобы не пропустить все изменения и идти в ногу с инструментом, не забывайте следить за блогом на официальном сайте.

Новый режим работы Blame Notifier позволяет "легковесно" выполнять часть функционала, который раньше был доступен в нашем продукте только в связке с SonarQube. Однако, хочется ещё раз повторить мы ни в коем случае не призываем отказываться от использования SonarQube. Новый режим позволяет лишь значительно проще попробовать такой подход к работе со статическим анализом. Мы верим, что для многих команд данный режим будет актуален.

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Maxim Stefanov. PVS-Studio New Features for Notifying Developers About Errors Found.

Подробнее..

Категории

Последние комментарии

  • Имя: Макс
    24.08.2022 | 11:28
    Я разраб в IT компании, работаю на арбитражную команду. Мы работаем с приламы и сайтами, при работе замечаются постоянные баны и лаги. Пацаны посоветовали сервис по анализу исходного кода,https://app Подробнее..
  • Имя: 9055410337
    20.08.2022 | 17:41
    поможем пишите в телеграм Подробнее..
  • Имя: sabbat
    17.08.2022 | 20:42
    Охренеть.. это просто шикарная статья, феноменально круто. Большое спасибо за разбор! Надеюсь как-нибудь с тобой связаться для обсуждений чего-либо) Подробнее..
  • Имя: Мария
    09.08.2022 | 14:44
    Добрый день. Если обладаете такой информацией, то подскажите, пожалуйста, где можно найти много-много материала по Yggdrasil и его уязвимостях для написания диплома? Благодарю. Подробнее..
© 2006-2024, personeltest.ru