Сейчас я изучаю отчёт очередной проверки проекта Chromium и используемых в нём библиотек, с помощью анализатора кода PVS-Studio. По итогам проверки у меня намечается цикл статей, посвященный разбору некоторых видов ошибок, и тому, как их можно избежать. Однако, одна ошибка так сильно понравилась, что я решил не откладывать её описание, а сразу написать маленькую заметку в блог.
Наша команда написала уже 5 статей (1, 2, 3, 4, 5), посвященных поиску ошибок в открытом проекте Chromium. И, видимо, скоро я напишу ещё несколько заметок в наш блог на эту тему.
Сейчас я изучаю отчёт, выданный анализатором PVS-Studio и просто выписываю найденные ошибки. Написание заметок — это уже следующий этап. Я люблю в начале просмотреть отчёт, а потом уже решаю, как и о чём лучше написать. Однако, одна ошибка мне особенно понравилась, и я решил описать её сразу, не откладывая на потом.
Ошибка живёт в библиотеке Protocol Buffers (protobuf), используемой в проекте Chromium. Protocol Buffers — это протокол сериализации (передачи) структурированных данных, предложенный Google как эффективная бинарная альтернатива текстовому формату XML.
Если бы я увидел эту ошибку пару месяцев назад, я бы прошел мимо неё. Ну ошибка и ошибка. Однако, увидев её, я сразу вспомнил эпический сбой кассовых аппаратов, который недавно произошел в России. 20 декабря крупнейшие ритейлеры и сети АЗС по всей России столкнулись со сбоями в работе новых кассовых аппаратов. Первыми о проблеме узнали жители Владивостока, далее она распространилась по стране по мере начала рабочего дня, затронув Новосибирск, Барнаул, Красноярск, Кемерово и другие крупные города.
Ошибка в кассовых аппаратах и ошибка в Protocol Buffers это две разные ошибки, никак не связанные между собой. Но мне захотелось показать, как могут возникать подобные ошибки. Ведь часто дефекты кроятся не в хитрых алгоритмах, а являются следствием банальных опечаток. Я не знаю, что там было напутано в коде касс, но зато знаю, как глупая опечатка приводит к неработоспособности функции ValidateDateTime, предназначенной для проверки даты в Protocol Buffers. Давайте посмотрим на код этой функции и разберёмся, в нём.
static const int kDaysInMonth[13] = {
0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
};
bool ValidateDateTime(const DateTime& time) {
if (time.year < 1 || time.year > 9999 ||
time.month < 1 || time.month > 12 ||
time.day < 1 || time.day > 31 ||
time.hour < 0 || time.hour > 23 ||
time.minute < 0 || time.minute > 59 ||
time.second < 0 || time.second > 59) {
return false;
}
if (time.month == 2 && IsLeapYear(time.year)) {
return time.month <= kDaysInMonth[time.month] + 1;
} else {
return time.month <= kDaysInMonth[time.month];
}
}Функция ValidateDateTime принимает на вход дату и должна определить, корректна она или нет. В начале выполняются основные проверки. Проверяется, что номер месяца лежит в диапазоне [1..12], дни находятся в диапазоне [1..31], минуты находятся в диапазоне [0..59] и так далее. В коде всё это хорошо видно, и не требует специальных пояснений.
Далее идёт более сложная проверка, есть ли определённый день в определённом месяце. Например, декабрь состоит из 31 дня, а в ноябре 31-ого число нет, так как в месяце только 30 дней.
Чтобы проверить дни и при этом не писать множество операторов if или длинный switch, используется вспомогательный массив kDaysInMonth. В массиве записано количество дней в различных месяцах. По номеру месяца из массива извлекается максимальное количество дней и используется для проверки.
Дополнительно учитывается, что год может быть високосным, и тогда в феврале на 1 день больше.
В общем функция написана просто и красиво. Вот только неправильно.
Из-за опечатки дни проверяются неправильно. Обратите внимание, что с максимально возможным количеством дней в месяце, сравнивается не день, а месяц.
Ещё раз взгляните на код:
if (time.month == 2 && IsLeapYear(time.year)) {
return time.month <= kDaysInMonth[time.month] + 1;
} else {
return time.month <= kDaysInMonth[time.month];
}В сравнениях "time.month <=" надо использовать не член структуры month, а член day. Т.е. корректный код должен выглядеть так:
if (time.month == 2 && IsLeapYear(time.year)) {
return time.day <= kDaysInMonth[time.month] + 1;
} else {
return time.day <= kDaysInMonth[time.month];
}Естественно номер месяца (от 1 до 12), всегда меньше количества дней в любом из месяцев.
Из-за этой ошибки корректными будут считаться такие дни, как, например, 31 февраля или 31 ноября.
Вот такая вот интересная ошибка встретилась. Из-за неё в обработку могут поступать некорректные даты, что в дальнейшем теоретически может использоваться для атаки системы. Тут я, пожалуй, сгущаю краски, но собственно так обычно и выглядят уязвимости. Просто где-то какие-то входные данные не проверены, и этим кто-то догадался воспользоваться.
Данная ошибка (вернее две ошибки) обнаруживается с помощью следующих предупреждений PVS-Studio:
- V547 / CWE-571 Expression 'time.month <= kDaysInMonth[time.month] + 1' is always true. time.cc 83
- V547 / CWE-571 Expression 'time.month <= kDaysInMonth[time.month]' is always true. time.cc 85
Как видите, теперь PVS-Studio также сразу классифицирует предупреждения согласно Common Weakness Enumeration (CWE).
Ещё хочу обратить внимание, что анализатор PVS-Studio всё более глубоко учится анализировать код. Сама по себе диагностика V547 старая (была реализована в 2010 году). Однако, скажем, год назад анализатор не смог бы найти эту ошибку. Теперь анализатор умеет заглянуть внутрь массива и понять, что извлекаются значения в диапазоне [28..31]. При этом он понимает, что 0 в массиве учитывать не надо, так как возможный диапазон time.month равен [1..12]. Если бы номер месяца был, скажем, равен 100, то произошел выход из функции и анализатор это понимает.
В итоге, анализатор видит, что происходят следующие сравнения диапазонов:
- [1..12] <= [28..31]
- [1..12] <= [29..32]
Следовательно, условия всегда истинны, о чём анализатор и выдаёт предупреждение. Вот такой вот глубокий анализ. Как видите, мы не только добавляем новые предупреждения в PVS-Studio, но и дорабатываем Data-Flow анализ, что сказывается на качестве уже существующих диагностик.
Следует задаться вопросом: как можно улучшить стиль, чтобы защититься от подобных ошибок?
Не знаю. Функция простая и написана в хорошем стиле. Просто человеку свойственно ошибаться, и он может допускать подобные опечатки. Ни один профессионал не застрахован от таких ошибок.
Можно только порекомендовать писать более внимательно юнит-тесты и использовать профессиональные статические анализаторы кода, такие как PVS-Studio.
Спасибо за внимание. Пойду дальше изучать отчёт.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. February 31.
Комментарии (95)
mikhaelkh
05.01.2018 23:16Andrey2008, попадали ли ошибки, найденные PVS Studio под программы Bug Bounty, или в базы наподобие CVE?
Ждёте ли Вы перед публикацией, пока ошибки проанализируют на возможность эксплутирования хотя бы в таких активных и критически важных проектах, как Chromium?
Насколько вообще реально с помощью статического анализатора типа PVS-Studio наткнуться на серьёзную проблему безопасности в открытых проектах?
extempl
06.01.2018 00:05+1таких активных и критически важных проектах
наткнуться на серьёзную проблему безопасности в открытых проектахНавряд ли. Серьёзные проблемы безопасности (уязвимости то бишь), это не опечатка в коде. Это целая цепочка сложных, на первый взгляд (часто) не связанных между собой шагов плюс уязвимости в third-party продуктах.
Если бы всё было так просто — любые уязвимости покрывались бы тестами :)
Andrey2008 Автор
06.01.2018 01:10+1Да, анализатор PVS-Studio может выявить уязвимости. Раз он выявляет большое количество CWE-дефектов, то часть из них вполне могут превращаться в CVE. Подробнее.
Я думаю, что иногда (сами того не зная) мы находим уязвимости и помогаем их устранить. Но у нас не хватает сил проводить более тщательное исследование по каждой ошибке, чтобы понять, является она уязвимостью или нет.
ufm
05.01.2018 23:26+4А то, что в минуте бывает 61 секунда — учитывать не надо?
А то, что «1 февраля 1918» на территории ExUSSR — это неверная дата?
Во всех книгах по криптографии написано — «не надо придумывать свою систему шифрования, потому что вы по определению напишете хуже и наделаете кучу ошибок». Вот с разбором дат надо такое-же везде писать.
datacompboy
06.01.2018 00:25Бывает только 60я секунда. Даже если в году две високосных секунды — между ними пол года будет
ufm
06.01.2018 00:41+2В 1 минуте обычно 60 секунд.
Leap second — 61-ая секунда, не поверите.
Для информации — например в 2016 году 31 декабря на часах было 23:59:60datacompboy
06.01.2018 00:44Ага. Мы о разном говорим. Я не про количество оных, а про индекс.
То есть секунда==60 валидна (в очень продвинутых либах), секунда равная 61 нет.
Просто это известная ошибка в библиотеке явы, которая позволяет секунду равную 61.
Но нет, насколько я понимаю, протобаф не жуёт високосные секунды.
kekekeks
06.01.2018 11:35секунда равная 61
Вы не поверите, но это 62-ая. Они на часах с нуля идут.
datacompboy
06.01.2018 17:06См.мой комментарий. Индекс vs количество. "первая секунда" не детерминированная фраза, может означать и нулевую и первую.
MacIn
06.01.2018 20:54+2Не поверим. Логично, что 1я — это между 00:00 и 00:01, 60я — между 00:59 и 01:00.
DistortNeo
06.01.2018 01:07+1Бывают случаи, когда в 1 минуте возможно не 60, а 64 секунды, да вообще, когда на входе задана невалидная дата, и с этой датой нужно полноценно работать, а не поднимать лапки.
Например, в FAT32 или ZIP-архивах можно назначить время создания файла 27:60:62. Будет весело, если программа, агрессивно валидирующая дату, будет падать на таких файлах.
Ну или когда контроллер телекоммуникационной стойки без тени сомнения говорит, что на его часах 12:13:60 — с этим тоже нужно уметь работать.datacompboy
06.01.2018 01:18Нет, это не в одной минуте, а где-то нужно хранение невалидных дат. Там надо подходить соответствующим образом для эмуляции багов
sabio
06.01.2018 01:33+2Заблуждения программистов относительно времени
https://habrahabr.ru/post/146109/
Больше заблуждений программистов относительно времени
https://habrahabr.ru/post/313274/
kirillaristov
06.01.2018 16:15Объясните, пожалуйста, откуда берется 61 (62,63,64) секунда? Все прочел, но так и не понял — ведь 1 минута = 60 секунд и это чуть ли не метрическая система единиц.
TimsTims
06.01.2018 17:02Вот с разбором дат надо такое-же везде писать.
Было бы хорошо, но есть такая интерасная штука, как время в формате PM/AM:
11:00 AM
12:00 AM
01:00 AM
02:00 AM
03:00 AM
…
10:00 AM
11:00 AM
12:00 PM
01:00 PM
02:00 PM…
Т.е. 12 PM почти всегда меньше 01 PM.
А 12 AM меньше 01 AM.
И всё бы хорошо, можно захардкодить, НО: в некоторых странах(Австралия), где то же принято использовать AM/PM, нумерация может отличаться, и идти логически правильно:
11 PM
12 PM
01 AM
Поэтому не получится «один раз написать» и забыть, надо еще страну как минимум учитывать)sasha1024
06.01.2018 17:48+1Ну, это уже, ИМХО, вопрос парсинга/генерации локализованных строковых форматов (немного другая тема), а не проверки валидности структуры для хранения.
То естьtime.dayпо-любому будет от 0 до 23, а вот то, что в локализованной строкеtime.day=12может записываться и как «12 AM», и как «12 PM» (в зависимости от страны), та и разделители/порядок могут отличаться — это отдельная тема.datacompboy
07.01.2018 08:50+1Time.hour а не .day ;))
sasha1024
07.01.2018 09:24Ой, ну да.
Я просто копировал из текста статьи дважды: первый раз Ctrl+C не отработало, а второй раз, видимо, моё внимание.
Lain_13
09.01.2018 11:42Хочу заметить, что это хоть и контринтуитивно, но 12 AM после 11 PM это именно что «логически правильно» так-как любой момент сразу после полуночи или полудня будет уже после, а сами по-себе полдень и полночь временной продолжительности не имеют. Если на то пошло, то идеально точное 12:00 будет просто meridiem без ante или post.
Я терпеть не могу, когда в 11:17 говорят «уже 12й час идёт», но ведь так оно и есть на самом деле и здесь то же самое. В 12:00 AM идёт первая минута до полудня.
ximaera
05.01.2018 23:45+2В кассовых аппаратах ничего не было напутано, там обиженный разработчик закладку сделал.
Karpion
06.01.2018 07:18В физике есть «теория размерностей», которая говорит, что килограммы нельзя сравнивать с метрами (хотя в общей теории поля размерности пропадают — например, метры переходят в секунды со скоростью света).
Так вот, в программировании давно нужны языки, в которых переменные имеют что-то типа размерности. Чтобы номер месяца и номер дня в месяце нельзя было ни складывать, ни сравнивать.
В языке Паскаль было что-то такое: например, можно было создать перечисляемые типы «месяц» и «день_недели»; их возможные значения выглядели как предопределённые константы, а в применении — контролировался тип.
maaGames
06.01.2018 10:48Просто вместо «int day» нужно использовать «class Day day». Или class enum Day (не помню синтаксис для строгих перечислений), но полноценный класс мне больше в данном случае нравится. Другое дело, что слишком много неудобств появляется, помимо кучи удобств.
lxsmkv
06.01.2018 12:18Когда я пытался понять как можно было бы избежать такой проблемы в общем, моя первая мысль тоже была, что нужно чтобы все данные имели разный тип,
чтобы например было time.year.isLeap(), time.month.isFebruary().
Вот в чем недостаток примитивных типов в яве. Не в том, что это плохо соотносится с идеей ооп, где все должно быть обьектом. Просто человеку проще сделать сравнение на примитивных числовых типах, чем писать сравнительные методы. Если по пожарной лестнице добраться до квартиры быстрее чем на лифте — это рано или поздно плохо кончится. Схожая история с динамически типизированными языками. Они дают возможность запилить пару багов которые всплывут только во время выполнения.
Ну и конечно много значит сам подход. Попытка быстро избавится от симптома, без особого стремления к качественному улучшению (да, качественное улучшение не тоже самое, что улучшение качества). О, дырка — заклеим. О, пятно — закрасим. О, грязь — подметем. Это же наша работа. А в дипломе написано инженер. Но разве инженер это в первую очередь не подход к решению проблем? Я пишу это как напоминание самому себе.
По работе мне приходится в основном писать тестировочный код и я пришел к тому, что в коде теста («клиенте») должен присутствовать минимум логики. В идеале вообще никакой. Вся логика в методах классов. Сам тест состоит из последовательных вызовов методов. Это повышает поддерживаемость («maintainability») и изменяемость(«changeability) кода. Можно из консоли создать обьекты и перед составлением теста пройти сценарий в шаговом режиме. Улучшается ортогональность: можно увеличивать количество классов и количество тестов независимо друг от друга. Распределять работу: один имплементирует, другой использует в месте назначения (клиенте).
Ну и про сложносоставные логические выражения. Это конечно круто выглядит. Больше сказать о них нечего.maaGames
06.01.2018 12:42Проблема даже в том, что если не сделать преобразование в/из примитивных типов, то пользоваться будет невозможно, а если сделать, пусть и явным, преобразование, то проблема чуть уменьшится, но никуда не денется.
Sirikid
07.01.2018 06:14Такие языки давно есть, просто C++ не один из них.
Вот, например, на Idris. Объявлен тип данных "день месяца" и доказана теорема (goal) "номер дня февраля всегда меньше 29":
Заголовок спойлераimport Data.Fin import Data.List Year : Type Leap : Year -> Type data Month = Jan | Feb | Mar | Apr | May | Jun | Jul | Avg | Sep | Oct | Nov | Dec Long : Month -> Type Long m = Elem m [Jan, Mar, May, Jul, Avg, Oct, Dec] Short : Month -> Type Short m = Elem m [Apr, Jun, Sep, Nov] data DayOfMonth : Year -> Month -> Type where LongMonthDay : (day : Fin 31) -> {auto pf : Long month} -> DayOfMonth _ month ShortMonthDay : (day : Fin 30) -> {auto pf : Short month} -> DayOfMonth _ month FebDay : (day : Fin 28) -> DayOfMonth _ Feb FebThe29th : {auto pf : Leap year} -> DayOfMonth year Feb domToNat : DayOfMonth y m -> Nat domToNat (LongMonthDay day) = finToNat day + 1 domToNat (ShortMonthDay day) = finToNat day + 1 domToNat (FebDay day) = finToNat day + 1 domToNat FebThe29th = 29 goal : (day : DayOfMonth _ Feb) -> LTE (domToNat day) 29 goal (LongMonthDay _ {pf}) = absurd (lemma pf) where lemma : Not (Long Feb) lemma (There (There (There (There (There (There (There Here))))))) impossible lemma (There (There (There (There (There (There (There (There _)))))))) impossible goal (ShortMonthDay _ {pf}) = absurd (lemma pf) where lemma : Not (Short Feb) lemma (There (There (There (There Here)))) impossible lemma (There (There (There (There (There _))))) impossible goal (FebDay day) = rewrite sym (plusSuccRightSucc (finToNat day) 0) in rewrite plusZeroRightNeutral (finToNat day) in LTESucc (lemma day) where lemma : (k : Fin n) -> LTE (finToNat k) n lemma FZ {n = Z} impossible lemma (FS _) {n = Z} impossible lemma FZ {n = S _} = LTEZero lemma (FS k) {n = S _} = LTESucc (lemma k) goal FebThe29th = lteReflKarpion
07.01.2018 19:47Фразу «в программировании давно нужны языки» надо понимать как «эти языки надо использовать», а не как «надо, чтобы эти языки просто были где-то в музее никогда не использовавшихся языков».
Sirikid
07.01.2018 23:55Используйте, кто ж вам не дает? Есть и примеры софта, написанного на таких языка, те же самолеты, и отчасти автомобили, обмазывают верификацией по самое небалуйся.
Karpion
08.01.2018 01:12Вы опять меня не поняли. Вопрос не в том, чтобы эти языки использовал я. Вопрос в том, чтобы языки без строжайшей типизации вообще не использовали в сложных и важных проектах. В идеале — хорошо бы запретить это гос.законом; но это нереально, т.к. гос.структуры работают сами знаете как: например, вместо того, чтобы уничтожить спамеров, принимают «закон Яровой».
Наверно, проблему надо решать на уровне корпораций, примерно так:
1) Разработчик письменно обязуется использовать языки со строжайшей типизацией.
2) Разработчик имеет право (и это право оговорено в обязательстве, про которое говорит первый пункт) использовать языки без строжайшей типизации. Однако, в этом случае он будет очень строго наказан (допустим, большими штрафами) в случае ошибок, которые не были бы возможны в языках со строжайшей типизацией.
Про автомобили сравнительно недавно сообщали, что там можно перехватить управление через FM-радиоприёмник — вот такая там верификация. Интересно, где там была допущена ошибка, и могла ли строжайшая типизация предотвратить эту уязвимость.
martin_wanderer
07.01.2018 21:31Так в любом языке, где есть User Defined Types — заводишь, и вперед. В той же Джаве для этого отлично подходят перечисления
grumegargler
06.01.2018 07:21Интересная находка, мощный у вас инструмент!
P.S. Мне кажется решение через if было бы не самым плохим:
if ( Month == 2 ) return Day <= 28 + isLeap (); else return Day <= ( Month < 8 ? 30 + Month % 2 : 31 - Month % 2 );africaunite
06.01.2018 08:59Опечатку в таком коде с еще большей вероятностью не заметил бы ни программист, ни PVS
grumegargler
06.01.2018 10:20+1Может и так, но это можно сказать про любой код. Определение дней в месяце легко вычисляется и логику можно сосредоточить в одном месте, а не глобальный массив с 13 элементами для 12 месяцев только невисокосного года, который еще 'доучитывается' в теле функции.
hdfan2
06.01.2018 09:44+19Слишком примитивно. Даёшь магию во все поля!
if ( Month == 2 ) return Day <= 28 + isLeap (); else return Day <= (int)(sin(Month * M_PI * 0.87) / 2 + 1) + 30;mikhaelkh
06.01.2018 23:42+1Даёшь магию во все поля!
Да пожалуйста:
return Day <= (((Month & 1) ^ (Month >> 3)) | 30) + (~(((Month - 2) | (2 - Month)) >> 4) & (isLeap(Year) - 2));DistortNeo
06.01.2018 23:47+2Можно и так:
return Day <= 28 + (0x3BBEECC + isLeap(Year) * 0x10 >> Month * 2 & 3);grumegargler
07.01.2018 02:44Парни, шутки-шутками, но определить, что число четное от 1 до 12 с поправкой на месяц…ну ведь это с++, а не c++script…
Не хочу критиковать, но и массив, с закладкой что месяц не индекс, плюс отдельная подработка внутри функции…неужели такое красивое решение?
hdfan2
07.01.2018 07:29Прекрасно! Вот только функция isLeap меня смущает. Слишком очевидно и легко сопровождаемо. Нужно избавиться и от неё. Естественно, всякие
это слишком примитивно. Можно остановиться на(Year % 4 == 0 && (Year % 100 != 0 || Year % 400 == 0) ? 1 : 0)
Но можно пойти ещё чуть дальше:!(Year & 3) - !(Year % 100) + !(Year % 400)
Тут у меня фантазия пока остановилась. Может, кто-нибудь придумает, как избавиться от этих слишком очевидных 100 и 400?(Year & 1 | (Year >> 1) & 1) ^ 1 - !(Year % 100) + !(Year % 400)
P.S. Можно, конечно, использовать и старые добрые синусы:
(int)(sin(-(Year - 1) * M_PI / 2) / 2 + .5) - (int)(sin(-(Year - 25) * M_PI / 50) / 2 + .5) + (sin(-(Year - 100) * M_PI / 200) / 2 + .5)
Но рядом с битовыми операциями выглядит несколько эклектично.mikhaelkh
07.01.2018 10:59Так как я хочу, чтобы мой код ещё и поняли, я не буду писать всё в одну строку.
static inline int f(int x) { // return x ? -1 : 0; static const int INT_BIT = CHAR_BIT*sizeof(int) - 1; return (x|-x) >> INT_BIT; } static inline int isLeap(int Year) { int YearDiv100 = Year * 1374389535LL >> 37; return 1 + (f(Year & 3) ^ f(Year - YearDiv100 * 100) ^ f(Year - (YearDiv100 >> 2) * 400)); }
P.S.
^ имеет приоритет ниже чем -, пропущены скобки(Year & 1 | (Year >> 1) & 1) ^ 1 - !(Year % 100) + !(Year % 400)
GerrAlt
06.01.2018 12:10+3return Day <= 28 + isLeap ();
не знаю насчет языка, для которого это написано, но вообще говоря isLeap() должен (вроде как из названия следует) возвращать boolean и применять к нему арифметическую операцию нехорошоdevalone
06.01.2018 12:13+1В C++ bool преобразуется в 0, если false и 1, если true
domix32
06.01.2018 22:12Если повезёт и булево значение не является кастомным дефайном. Ну и гарантировать что преобразованное значение компилятор точно превратит что-то такое в правильное значение
bool isLeap() { return bool(0xDEADBEEF); }devalone
07.01.2018 02:24Если повезёт и булево значение не является кастомным дефайном
Это тогда уже не булево значение.
Ну и гарантировать что преобразованное значение компилятор точно превратит что-то такое в правильное значение
bool isLeap() { return bool(0xDEADBEEF); }
Точно превратит, стандарт говорит, что 0xDEADBEEF преобразуется в true, а true преобразуется в 1.
ainoneko
07.01.2018 07:50Точно превратит, стандарт говорит, что 0xDEADBEEF преобразуется в true, а true преобразуется в 1.
Если у нас действительно C++, а не C, где не было «настоящего логического типа», так что false == 0, а про true было известно только, что оно не равно нулю.
(Если, например, isLeap() берётся из какой-то старой библиотеки, то там всякого можно ожидать.)
alexey-m-ukolov
06.01.2018 12:21Если интерпретировать выражение «не самым плохим» буквально, то тут я согласен, можно написать ещё хуже (чуть выше есть примеры) :)
JaroslavB
06.01.2018 14:26Кстати Protocol Buffers (protobuf) используеться в TensorFlow, возможно из-за бага с валидацией даты, моя нейросеть не может найти мне девушку…
Я уверен что если проанализировать код самого TensorFlow то найдуться ошибки покритичней, ибо чуствуеться еще сыроватость проэкта.Andrey2008 Автор
06.01.2018 14:26JaroslavB
06.01.2018 21:42Благодарю, интересная статья, если выходит что само C++ ядро у TensorFlow стабильное значит Python обертка не отшлифованная. Я недавно выхвачивал segmentation fault только из-за того что переименовал имя checkpoint файла из “model.data-00000-of-00001” в “model”. Я уже собирался анализировать core дампы с помощью GDB или побайтно смотреть diff-ы checkpoint файлов. Но потом понял что просто Machine Learning Python программисты не любят мирские входные проверки.
devalone
06.01.2018 14:38возможно из-за бага с валидацией даты, моя нейросеть не может найти мне девушку…
Подозреваю, что проблема не в нейросети :)
lany
06.01.2018 17:16+4Молодцы ребята, серьёзно :-) Я прокачиваю dataflow для Java в IntelliJ IDEA. Когда IDEA подобное находит, я пищу от радости. У нас, к сожалению, нет ни чтения элемента массива после инициализации, ни выполнения сложения на интервалах. Чтение конкретного элемента в принципе несложно поддержать (частично уже есть, скажем,
if(idx == 1) a[idx] = 5; <...> if(a[1] > 0)подсветит). Создание интервала значений по интервалу индексов интересная мысль, вроде тоже несложно, просто в голову не приходило, что это может такую пользу принести. А вот делать сложение/вычитание на интервалах я пока опасаюсь, потому что состояния опять будут расходиться, если операция в цикле, и надо будет с этим бороться. Плюс у нас анализ в онлайн-режиме, при редактировании кода, поэтому мы несколько ограничены в ресурсах (все и так жалуются, что Идея тормозит =)). Я пока поддержал только битовое И и остаток от деления, потому что эти операции могут существенно сжать интервал значений и расхождений в циклах с ними не наблюдается. Остаток от деления выловил, например, смешной баг вродеif(count % 10 == 11).
ppiskun
06.01.2018 18:53+1Товарищи, исправили уже нарушения GNU GPL? Те, связанные со статической линковкой glibc?
Сами не планируете выпуститься под GNU GPL?Andrey2008 Автор
06.01.2018 19:53Мы не планируем выпускаться под открытыми лицензиями. Сам продукт соблюдает все лицензии с момента первого релиза.
janatem
06.01.2018 19:22Всё-таки ошибка возникла не при написании этого кода, а несколько раньше — при планировании проекта. Такие вещи как представление время, часовые пояса, календарь (которые несут в себе не столько сложную функциональность, сколько стандарты, договоренности и культурные особенности) должны находиться только в базовой библиотеке (такой как libc в Линуксе), которую использует миллиард пользователей и просматривает миллион пар глаз. Когда каждый проект начинает городить свои поделки, ошибки статистически неизбежны.
Здесь еще повезло, что PVS заметила странное сравнение, а если бы где-то вместо 30 было написано 31, то вот ну никто бы не помог, потому что код формально верен и имеет смысл, но несет в себе альтернативный стандарт.
lany
07.01.2018 14:16Неочевидно, хорошо ли это. Если это реализация строгой спецификации, то лучше навелосипедить, чем полагаться на стороннюю библиотеку и потом напороться на различия в деталях. Например, java.time при всей своей детальной проработанности официально по спецификации не учитывает високосные секунды, допускает годы от -999,999,999 до +999,999,999 и считает, что календарь всегда был Грегорианским (например, отбросит 29 февраля 1400 года как некорректную дату). Соответствует ли это вашей спецификации? Деталей может быть много и всегда есть шанс, что сторонняя реализация их трактует не так, как требуется вам.
janatem
07.01.2018 16:18Если java.time действительно так себя ведет, то ее реализация как раз нарушает основной принцип единственности источника спецификации.
Деталей может быть много и всегда есть шанс, что сторонняя реализация их трактует не так, как требуется вам.
Календарь и время — это как раз тот случай, когда не нужно хотеть своей спецификации. То есть своя реализация стандартной спецификации — это еще полбеды (там могут вкрасться трудноуловимые ошибки, которые когда-нибудь можно исправить), но вот своя спецификация — это совсем за гранью.
Andrey2008 Автор
07.01.2018 12:28+1Кстати, вдруг вспомнил, что в своё время встретили VirtualBox другую разновидность бага работы с днями.
static const uint8_t g_acDaysInMonths[12] = { /*Jan Feb Mar Arp May Jun Jul Aug Sep Oct Nov Dec */ 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }; static const uint8_t g_acDaysInMonthsLeap[12] = { /*Jan Feb Mar Arp May Jun Jul Aug Sep Oct Nov Dec */ 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }; static PRTTIME rtTimeNormalizeInternal(PRTTIME pTime) { .... unsigned cDaysInMonth = fLeapYear ? g_acDaysInMonthsLeap[pTime->u8Month - 1] // <= : g_acDaysInMonthsLeap[pTime->u8Month - 1]; // <= .... }
V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: g_acDaysInMonthsLeap[pTime->u8Month — 1]. time.cpp 453
datacompboy
Спасибо! Сейчас пофиксю.
vassabi
вот это скорость!
PS: Нескромный вопрос: а у вас в отделе используются какие-нибудь статические анализаторы, или как обычно: все проверки — глазами код-ревьювера?
datacompboy
есть статические анализаторы. я пытался прикрутить PVS, сломался на интеграции в некоторых местах.
времени нет, хочу сделать еще одну попытку — после чего выбить соответствующее финансирование, чтоб хотя бы на год в нашей подветке его потестить…
EvgeniyRyzhkov
С проектами типа Chromium никакие инструменты из коробки не работают. Это не моя мысль — это говорили ребята из московской разработки Google, пока ее не разогнали.
Это я к чему. Нет смысла самому прикручивать пытаться, надо нам написать и все подскажем. Судя по количеству наших статей про Chromium, все-таки PVS-Studio прикручивается :-).
datacompboy
Ну проверку-то разово я тогда прогнал и даже два бага пофиксил. Вот только шума слишком много, и не автоматизировано, надо-то чтоб оно само. В общем, пару выходных я все же убью ещё раз, благо прошлые черновики сохранились.
И обязательно ещё за ключиком схожу ;) дабы демонстрация работала…
Andrey2008 Автор
Да, такое бывает. При этом, не надо героически преодолевать шум. Надо настраивать анализатор. Например, на Chromuim половина! всех ложных срабатываний связана с макросом DCHECK. Если только один этот макрос доработать напильником, жизнь уже сильно улучшится. Я про это скоро напишу.
datacompboy
И в мыслях не было :)
Шум надо устранять возле его появления, иначе инструмент только мешает, фильтры для известных вещей, и т.п. — ежу понятно.
Макросы одна беда, вторая — местоположение дефолтных библиотек, которые точно надо игнорировать.
kafeman
Неужели парсер / валидатор не покрыт тестами? Тут они прям так и просятся.
datacompboy
покрыты. и тесты на валидацию есть. только почему-то исключительно валидные случаи проверялись — со всеми вытекающими :)
mantyr
А что если дата локальная и 60 +1 или +2 секунды в какой-то момент это норма?:)
datacompboy
Основа формата — число секунд с точки Х. Не зря там в тестах после заполнения структуры конвертирует туда и обратно.
Поэтому изначально произвольный таймстамп невозможен
Stalker_RED
61 секунда в минуте — это редкая, но нормальная ситуация.
datacompboy
github.com/google/protobuf/pull/4147 — фикс
github.com/google/protobuf/pull/4148 — дополнительные тесты
splav_asv
А это такой стиль принят в protobuf, что код и доп тесты в отдельных PR идут? Может не обращал внимание, но вроде бы не попадалось. И если это так сделано намеренно, то в чём основной аргумент за?
datacompboy
В первую очередь — увидеть что тесты падают до фикса и не падают после.
У меня на рабочем лаптопе нет ключа от персонального гитхаба, поэтому редактировал онлайн :D
зы: я не настоящий сварщик, я работаю над BigQuery.
alexey-m-ukolov
А в чём смысл двойной проверки Non-leap year и Leap year?
datacompboy
У високосного года есть не только "делится на 4" но ещё условия
alexey-m-ukolov
Но ведь эти тесты не для проверки работы IsLeapYear, по идее? Вот там эти проверки были бы уместны (и они там есть), а здесь они только вопросы вызывают — разве поведение этой функции меняется в зависимости от того, какой именно високосный год ей дадут?
datacompboy
Нет, по крайней мере сейчас. Но это один из антипаттернов — тестировать код, зная как он устроен. Такие тесты ломаются или не ловят поломки при рефакторинге или смене алгоритма.
Мои тесты тоже не верх качества, так как в идеале надо проверять на все месяцы независимо.
alexey-m-ukolov
Спасибо, сообразил.
Andrey2008 Автор
Good. Я скоро ещё принесу. Около 250 штук. :)
datacompboy
Врядли я этим займусь, я лучше время на интеграцию ещё раз потрачу, если смогу выкроить :)
Пользы будет больше…