Браузер Chromium очень быстро развивается. Например, когда в 2011 году мы впервые проверили этот проект (solution), он состоял из 473 проектов. Сейчас, он состоит уже из 1169 проектов. Нам было интересно, смогли ли разработчики Google сохранить высочайшее качество кода, при такой скорости развития Chromium. Да, смогли.
Chromium
Chromium — веб-браузер с открытым исходным кодом, разработанный компанией Google. На основе Chromium создаётся браузер Google Chrome. На странице "Get the Code" можно узнать, как скачать исходный код этого проекта.
Немного общей информации
Раньше мы уже проверяли проект Chromium, о чём имеется две статьи: первая проверка (23.05.2011), вторая проверка (13.10.2011). И всё время находили ошибки. Это тонкий намёк о пользе анализаторов кода.
Сейчас (исходный код проекта скачен в июле 2013) Chromium состоит из 1169 проектов. Общий объем исходного кода на языке Си/Си++ составляет 260 мегабайт. Дополнительно к этому можно прибавить ещё 450 мегабайт используемых внешних библиотек.
Если взять нашу первую проверку проекта Chromium в 2011 году, то можно заметить — объем внешних библиотек в целом не изменился. Зато код, самого проекта существенно вырос c 155 мегабайт до 260 мегабайт.
Ради интереса посчитаем цикломатическую сложность
В анализаторе PVS-Studio есть возможность поиска функций с большой цикломатической сложностью. Как правило, такие функции являются кандидатами для рефакторинга. Проверив 1160 проектов, мне естественно стало интересно, какой из них можно назвать рекордсменом в номинации «самая сложная функция».
Самая максимальная цикломатическая сложность, равная 2782, принадлежит функции ValidateChunkAMD64() в проекте Chromium. Но её пришлось дисквалифицировать из состязания. Функция находится в файле validator_x86_64.c, который является автогенерируемым. Жаль. А то был бы эпичный рекордсмен. Я и близко с такой цикломатической сложностью не сталкивался.
Таким образом, первые три места получают следующие функции:
- Библиотека WebKit. Функция HTMLTokenizer::nextToken() в файле htmltokenizer.cpp. Цикломатическая сложность 1106.
- Библиотека Mesa. Функция _mesa_glsl_lex() в файле glsl_lexer.cc. Цикломатическая сложность 1088.
- Библиотека usrsctplib (какой-то безызвестный спортсмен). Функция sctp_setopt() в файле htmltokenizer.cpp. Цикломатическая сложность 1026.
Если кто-то не знает, что такое цикломатическая сложность 1000, то пусть и не знает. Психическое здоровье будет лучше :). В общем, много это.
Качество кода
Что я могу сказать о качестве кода проекта Chromium? Качество по-прежнему великолепное. Да, как и в любом большом проекте, всегда можно найти ошибки. Но если поделить их количество на объем кода, их плотность будет ничтожна. Это очень хороший код с очень малым количеством ошибок. Вручаю медальку за чистый код. Предыдущая медалька досталась проекту Casablanca (C++ REST SDK) от компании Mcorosoft.
Рисунок 1. Медалька создателям Chromium.
За компанию вместе с Chromium были проверенные входящие в него сторонние библиотеки. Но описывать найденные в них ошибки не интересно. Тем более я просматривал отчёт очень поверхностно. Нет, я вовсе не плохой человек. Я бы посмотрел на вас, если бы вы попробовали полноценно изучить отчёт о проверке 1169 проектов. То, что я заметил при беглом просмотре, я поместил в базу примеров ошибок. В этой статье я хочу коснуться только тех ошибок, которые успел заметить в коде самого Chromium (его плагинов и тому подобного).
Раз проект Chromium, такой хороший, так зачем я буду приводить примеры найденных ошибок? Всё очень просто. Я хочу продемонстрировать мощь анализатора PVS-Studio. Если он сумел найти ошибки в Chromium, то инструмент заслуживает вашего внимания.
Анализатор сумел прожевать десятки тысяч файлов, общим объемом 710 мегабайт, и не загнулся от этого. Не смотря на то, что проект разрабатывается высококвалифицированными разработчиками и проверяется различными инструментами, PVS-Studio всё равно умудрился выявить дефекты. Это замечательное достижение! И последнее — он сделал это за разумное время (около 5 часов) за счёт параллельной проверки (AMD FX-8320/3.50 GHz/eight-core processor, 16.0 GB RAM).
Некоторые из найденных ошибок
Предлагаю рассмотреть некоторые примеры кода, на которых остановился мой взгляд при просмотре отчета. Уверен, что при детальном изучении, можно будет найти намного больше интересного.
Замеченное N1 — опечатки
Vector3dF
Matrix3F::SolveEigenproblem(Matrix3F* eigenvectors) const
{
// The matrix must be symmetric.
const float epsilon = std::numeric_limits<float>::epsilon();
if (std::abs(data_[M01] - data_[M10]) > epsilon ||
std::abs(data_[M02] - data_[M02]) > epsilon ||
std::abs(data_[M12] - data_[M21]) > epsilon) {
NOTREACHED();
return Vector3dF();
}
....
}V501 There are identical sub-expressions to the left and to the right of the '-' operator: data_[M02] — data_[M02] matrix3_f.cc 128
Надо проверить, что матрица размером 3x3 симметрична.
Рисунок 2. Матрица 3x3.
Для этого надо сравнить следующие элементы:
- M01 и M10
- M02 и M20
- M12 и M21
Скорее всего, код писался с помощью технологии Copy-Paste. В результате ячейка M02 сравнивается сама с собой. Вот такой вот забавный класс матрицы.
Другая простая опечатка:
bool IsTextField(const FormFieldData& field) {
return
field.form_control_type == "text" ||
field.form_control_type == "search" ||
field.form_control_type == "tel" ||
field.form_control_type == "url" ||
field.form_control_type == "email" ||
field.form_control_type == "text";
}V501 There are identical sub-expressions 'field.form_control_type == «text»' to the left and to the right of the '||' operator. autocomplete_history_manager.cc 35
Два раза происходит сравнение со строкой «text». Это подозрительно. Возможно, одна строка просто лишняя. А может быть, отсутствует другое нужное здесь сравнение.
Замеченное N2 — противоположные условия
static void ParseRequestCookieLine(
const std::string& header_value,
ParsedRequestCookies* parsed_cookies)
{
std::string::const_iterator i = header_value.begin();
....
if (*i == '"') {
while (i != header_value.end() && *i != '"') ++i;
....
}V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 500, 501. web_request_api_helpers.cc 500
Мне кажется, этот код должен был пропускать текст, обрамленный двойными кавычками. Но на самом деле, этот код ничего не делает. Условие сразу ложно. Для наглядности, напишу псевдокод, чтобы подчеркнуть суть ошибки:
if ( A == 'X' ) {
while ( .... && A != 'X' ) ....;Скорее всего, здесь забыли сдвинуть указатель на один символ и код должен выглядеть так:
if (*i == '"') {
++i;
while (i != header_value.end() && *i != '"') ++i;Замеченное N3 — неудачное удаление элементов
void ShortcutsProvider::DeleteMatchesWithURLs(
const std::set<GURL>& urls)
{
std::remove_if(matches_.begin(),
matches_.end(),
RemoveMatchPredicate(urls));
listener_->OnProviderUpdate(true);
}V530 The return value of function 'remove_if' is required to be utilized. shortcuts_provider.cc 136
Для удаления элементов из контейнера используется функция std::remove_if(). Но используется неправильно. На самом деле, remove_if() ничего не удаляет. Она сдвигает элементы в начало и возвращает итератор на мусор. Удалять мусор нужно самостоятельно, вызывая у контейнеров функцию erase(). См. также статью в Wikipedia "Erase-remove idiom".
Правильный код:
matches_.erase(std::remove_if(.....), matches_.end());Замеченное N4 — вечная путаница с SOCKET
SOCKET в мире Linux, это целочисленный ЗНАКОВЫЙ тип данных.
SOCKET в мире Windows, это целочисленный БЕЗЗНАКОВЫЙ тип данных.
В заголовочных файлах Visual C++ тип SOCKET объявлен так:
typedef UINT_PTR SOCKET;Однако про это постоянно забывают и пишут код следующего вида:
class NET_EXPORT_PRIVATE TCPServerSocketWin {
....
SOCKET socket_;
....
};
int TCPServerSocketWin::Listen(....) {
....
socket_ = socket(address.GetSockAddrFamily(),
SOCK_STREAM, IPPROTO_TCP);
if (socket_ < 0) {
PLOG(ERROR) << "socket() returned an error";
return MapSystemError(WSAGetLastError());
}
....
}V547 Expression 'socket_ < 0' is always false. Unsigned type value is never < 0. tcp_server_socket_win.cc 48
Переменная беззнакового типа всегда больше или равна нулю. Это значит, что проверка 'socket_ < 0' не имеет смысла. Если при работе программы сокет не удастся открыть, эта ситуация не будет корректно обработана.
Замеченное N5 — путаница с операциями ~ и !
enum FontStyle {
NORMAL = 0,
BOLD = 1,
ITALIC = 2,
UNDERLINE = 4,
};
void LabelButton::SetIsDefault(bool is_default) {
....
style = is_default ? style | gfx::Font::BOLD :
style & !gfx::Font::BOLD;
....
}V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. label_button.cc 131
Как мне кажется, код должен был работать так:
- Если переменная 'is_default' равна true, то всегда нужно выставить бит, отвечающий за стиль BOLD.
- Если переменная 'is_default' равна false, то всегда нужно сбросить бит, отвечающий за стиль BOLD.
Однако, выражение «style & !gfx::Font::BOLD» работает не так, как ожидает программист. Результат операции "!gfx::Font::BOLD" будет равен 'false'. Или другими словами, 0. Код написанный выше эквивалентен этому:
style = is_default ? style | gfx::Font::BOLD : 0;Чтобы код работал правильно, следовало использовать операцию '~':
style = is_default ? style | gfx::Font::BOLD :
style & ~gfx::Font::BOLD;Замеченное N6 — подозрительное создание временных объектов
base::win::ScopedComPtr<IDirect3DSurface9> scaler_scratch_surfaces_[2];
bool AcceleratedSurfaceTransformer::ResizeBilinear(
IDirect3DSurface9* src_surface, ....)
{
....
IDirect3DSurface9* read_buffer = (i == 0) ?
src_surface : scaler_scratch_surfaces_[read_buffer_index];
....
}V623 Consider inspecting the '?:' operator. A temporary object of the 'ScopedComPtr' type is being created and subsequently destroyed. Check second operand. accelerated_surface_transformer_win.cc 391
Вряд ли этот код привести к ошибке, но он заслуживает, чтобы рассказать о нём. Мне кажется, некоторые программисты узнают о новой интересной ловушке в языке Си++.
На первый взгляд здесь всё просто. В зависимости от условия, мы выбираем указатель 'src_surface' или один из элементов массива 'scaler_scratch_surfaces_'. Массив состоит из объектов типа base::win::ScopedComPtr<IDirect3DSurface9>, которые могут автоматически приводиться к указателю на IDirect3DSurface9.
Дьявол кроется в деталях.
Тернарный оператор '?:' не может возвращать разный тип в зависимости от условия. Поясню на простом примере.
int A = 1;
auto X = v ? A : 2.0;Оператор ?: возвращает тип 'double'. В результате переменная 'X' будет тоже иметь тип double. Но это не важно. Важно, что переменная 'A' будет неявно расширена до типа 'double'!
Беда возникает, если написать что-то подобное:
CString s1(L"1");
wchar_t s2[] = L"2";
bool a = false;
const wchar_t *s = a ? s1 : s2;В результате выполнения этого кода переменная 's' будет указывать на данные, находящиеся внутри временного объекта типа CString. Проблема в том, что этот объект будет сразу уничтожен.
Вернемся теперь к исходному коду Chromium.
IDirect3DSurface9* read_buffer = (i == 0) ?
src_surface : scaler_scratch_surfaces_[read_buffer_index];Здесь произойдет следующее, если выполнится условие 'i == 0':
- из указателя 'src_surface' будет сконструирован временный объект типа base::win::ScopedComPtr<IDirect3DSurface9>;
- временный объект будет неявно приведён к указателю типа IDirect3DSurface9 и помещён в переменную read_buffer;
- временный объект будет разрушен.
Я не знаю логику работы программы и класса ScopedComPtr и затрудняюсь сказать, могут ли возникнуть негативные последствия или нет. Скорее всего, в конструкторе счётчик количества ссылок будет увеличен, а в деструкторе уменьшен. И всё будет хорошо.
Если это не так, то ненароком можно получить не валидный указатель или сломать подсчёт ссылок.
Одним словом, даже если ошибки нет, буду рад, если читатели узнали что-то новое. Тернарный оператор куда опаснее, чем кажется.
Вот ещё одно такое подозрительное место:
typedef
GenericScopedHandle<HandleTraits, VerifierTraits> ScopedHandle;
DWORD HandlePolicy::DuplicateHandleProxyAction(....)
{
....
base::win::ScopedHandle remote_target_process;
....
HANDLE target_process =
remote_target_process.IsValid() ?
remote_target_process : ::GetCurrentProcess();
....
}V623 Consider inspecting the '?:' operator. A temporary object of the 'GenericScopedHandle' type is being created and subsequently destroyed. Check third operand. handle_policy.cc 81
Замеченное N7 — повторяющиеся проверки
string16 GetAccessString(HandleType handle_type,
ACCESS_MASK access) {
....
if (access & FILE_WRITE_ATTRIBUTES)
output.append(ASCIIToUTF16("tFILE_WRITE_ATTRIBUTESn"));
if (access & FILE_WRITE_DATA)
output.append(ASCIIToUTF16("tFILE_WRITE_DATAn"));
if (access & FILE_WRITE_EA)
output.append(ASCIIToUTF16("tFILE_WRITE_EAn"));
if (access & FILE_WRITE_EA)
output.append(ASCIIToUTF16("tFILE_WRITE_EAn"));
....
}V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 176, 178. handle_enumerator_win.cc 178
Если установлен флаг FILE_WRITE_EA, то стока "tFILE_WRITE_EAn" будет добавлена дважды. Очень подозрительный код.
Аналогичную странную картину можно наблюдать здесь:
static bool PasswordFormComparator(const PasswordForm& pf1,
const PasswordForm& pf2) {
if (pf1.submit_element < pf2.submit_element)
return true;
if (pf1.username_element < pf2.username_element)
return true;
if (pf1.username_value < pf2.username_value)
return true;
if (pf1.username_value < pf2.username_value)
return true;
if (pf1.password_element < pf2.password_element)
return true;
if (pf1.password_value < pf2.password_value)
return true;
return false;
}V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 259, 261. profile_sync_service_password_unittest.cc 261
Два раза повторяется проверка «pf1.username_value < pf2.username_value». Возможна, одна строка просто лишняя. А возможно, забыли проверить что-то другое, и должно быть написано совсем другое условие.
Замеченное N8 — «одноразовые» циклы
ResourceProvider::ResourceId
PictureLayerImpl::ContentsResourceId() const
{
....
for (PictureLayerTilingSet::CoverageIterator iter(....);
iter;
++iter)
{
if (!*iter)
return 0;
const ManagedTileState::TileVersion& tile_version = ....;
if (....)
return 0;
if (iter.geometry_rect() != content_rect)
return 0;
return tile_version.get_resource_id();
}
return 0;
}V612 An unconditional 'return' within a loop. picture_layer_impl.cc 638
С этим циклом что-то не так. Цикл выполняет только одну итерацию. В конце цикла находится безусловный оператор return. Возможные причины:
- Так и задумано. Но это очень сомнительно. Зачем тогда создавать цикл, создавать итератор и т.д.
- Вместо одного из 'return', должно быть написано 'continue'. Но тоже сомнительно.
- Скорее всего, перед последним 'return' пропущено какое-то условие.
Попались и другие странный циклы, которые выполняются только раз:
scoped_ptr<ActionInfo> ActionInfo::Load(....)
{
....
for (base::ListValue::const_iterator iter = icons->begin();
iter != icons->end(); ++iter)
{
std::string path;
if (....);
return scoped_ptr<ActionInfo>();
}
result->default_icon.Add(....);
break;
}
....
}V612 An unconditional 'break' within a loop. action_info.cc 76
const BluetoothServiceRecord* BluetoothDeviceWin::GetServiceRecord(
const std::string& uuid) const
{
for (ServiceRecordList::const_iterator iter =
service_record_list_.begin();
iter != service_record_list_.end();
++iter)
{
return *iter;
}
return NULL;
}V612 An unconditional 'return' within a loop. bluetooth_device_win.cc 224
Замеченное N9 — неинициализированные переменные
HRESULT IEEventSink::Attach(IWebBrowser2* browser) {
DCHECK(browser);
HRESULT result;
if (browser) {
web_browser2_ = browser;
FindIEProcessId();
result = DispEventAdvise(web_browser2_, &DIID_DWebBrowserEvents2);
}
return result;
}V614 Potentially uninitialized variable 'result' used. ie_event_sink.cc 240
Если указатель 'browser' равен нулю, то функция вернет неинициализированную переменную.
Другой фрагмент кода:
void SavePackage::GetSaveInfo() {
....
bool skip_dir_check;
....
if (....) {
....->GetSaveDir(...., &skip_dir_check);
}
....
BrowserThread::PostTask(BrowserThread::FILE,
FROM_HERE,
base::Bind(..., skip_dir_check, ...));
}V614 Potentially uninitialized variable 'skip_dir_check' used. Consider checking the fifth actual argument of the 'Bind' function. save_package.cc 1326
Переменная 'skip_dir_check' может остаться неинициализированной.
Замеченное N10 — выравнивание кода не соответствует логике его работы
void OnTraceNotification(int notification) {
if (notification & TraceLog::EVENT_WATCH_NOTIFICATION)
++event_watch_notification_;
notifications_received_ |= notification;
}V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. trace_event_unittest.cc 57
Рассматривая такой код, непонятно, забыты здесь фигурные скобки или нет. Даже если код корректен, его следует поправить, чтобы он не вводил других программистов в задумчивое состояние.
Вот ещё пара мест с ОЧЕНЬ подозрительным выравниванием кода:
- nss_memio.c 152
- nss_memio.c 184
Замеченное N11 — проверка указателя после new
Во многих программах живет старый унаследованный код, написанный ещё в те времена, когда оператор 'new' не кидал исключение. Раньше в случае нехватки памяти он возвращал нулевой указатель.
Chromium в этом плане не исключение, и в нем встречаются такие проверки. Беда не в том, что выполняется бессмысленная проверка. Опасно, что при нулевом указателе раньше должны были выполняться какие-то действия или функции должны возвращать определенные значения. Теперь из-за генерации исключения логика работы изменилась. Код, который должен был получить управление при ошибке выделения памяти, теперь бездействует.
Рассмотрим пример:
static base::DictionaryValue* GetDictValueStats(
const webrtc::StatsReport& report)
{
....
DictionaryValue* dict = new base::DictionaryValue();
if (!dict)
return NULL;
dict->SetDouble("timestamp", report.timestamp);
base::ListValue* values = new base::ListValue();
if (!values) {
delete dict;
return NULL;
}
....
}V668 There is no sense in testing the 'dict' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 164
V668 There is no sense in testing the 'values' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 169
Первая проверка «if (!dict) return NULL;» скорее всего вреда не принесёт. А вот со второй проверкой дело обстоит хуже. Если при создании объекта с помощью «new base::ListValue()» не удастся выделить память, то будет сгенерировано исключение 'std::bad_alloc'. На этом работа функции GetDictValueStats() завершится.
В результате, вот этот код:
if (!values) {
delete dict;
return NULL;
}никогда не уничтожит объект, адрес которого хранится в переменной 'dict'.
Правильным решением здесь будет проведение рефакторинга кода и использование умных указателей (smart pointers).
Рассмотрим другой фрагмент кода:
bool Target::Init() {
{
....
ctx_ = new uint8_t[abi_->GetContextSize()];
if (NULL == ctx_) {
Destroy();
return false;
}
....
}V668 There is no sense in testing the 'ctx_' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. target.cc 73
В случае ошибки выделения памяти не будет вызвана функция Destroy().
Дальше писать про это не интересно. Я просто приведу список других замеченных мной потенциально опасных мест в коде:
- 'data' pointer. target.cc 109
- 'page_data' pointer. mock_printer.cc 229
- 'module' pointer. pepper_entrypoints.cc 39
- 'c_protocols' pointer. websocket.cc 44
- 'type_enum' pointer. pin_base_win.cc 96
- 'pin_enum' pointer. filter_base_win.cc 75
- 'port_data'. port_monitor.cc 388
- 'xcv_data' pointer. port_monitor.cc 552
- 'monitor_data'. port_monitor.cc 625
- 'sender_' pointer. crash_service.cc 221
- 'cache' pointer. crash_cache.cc 269
- 'current_browser' pointer. print_preview_dialog_controller.cc 403
- 'udp_socket' pointer. network_stats.cc 212
- 'popup_' pointer. try_chrome_dialog_view.cc 90
Замеченное N12 — тесты, которые плохо тестируют
Юнит тесты замечательная технология повышения качества программ. Однако сами тесты нередко содержат ошибки, в результате чего не выполняют свои функции. Писать тесты для тестов это конечно перебор. Помочь может статический анализ кода. Подробнее эту мысль я рассматривал в статье "Как статический анализ дополняет TDD".
Приведу некоторые примеры ошибок, встретившиеся мне в тестах для Chromium:
std::string TestAudioConfig::TestValidConfigs() {
....
static const uint32_t kRequestFrameCounts[] = {
PP_AUDIOMINSAMPLEFRAMECOUNT,
PP_AUDIOMAXSAMPLEFRAMECOUNT,
1024,
2048,
4096
};
....
for (size_t j = 0;
j < sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts);
j++) {
....
}V501 There are identical sub-expressions 'sizeof (kRequestFrameCounts)' to the left and to the right of the '/' operator. test_audio_config.cc 56
В цикле выполнятся только один тест. Ошибка в том, что «sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts)» равняется единице. Правильное выражение: «sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts[0])».
Другой ошибочный тест:
void DiskCacheEntryTest::ExternalSyncIOBackground(....) {
....
scoped_refptr<net::IOBuffer> buffer1(new net::IOBuffer(kSize1));
scoped_refptr<net::IOBuffer> buffer2(new net::IOBuffer(kSize2));
....
EXPECT_EQ(0, memcmp(buffer2->data(), buffer2->data(), 10000));
....
}V549 The first argument of 'memcmp' function is equal to the second argument. entry_unittest.cc 393
Функция «memcmp()» сравнивает буфер сам с собой. В результате тест не выполняет требуемую проверку. Видимо, здесь должно быть:
EXPECT_EQ(0, memcmp(buffer1->data(), buffer2->data(), 10000));А вот тест, который может неожиданно сломать другие тесты:
static const int kNumPainters = 3;
static const struct {
const char* name;
GPUPainter* painter;
} painters[] = {
{ "CPU CSC + GPU Render", new CPUColorPainter() },
{ "GPU CSC/Render", new GPUColorWithLuminancePainter() },
};
int main(int argc, char** argv) {
....
// Run GPU painter tests.
for (int i = 0; i < kNumPainters; i++) {
scoped_ptr<GPUPainter> painter(painters[i].painter);
....
}V557 Array overrun is possible. The value of 'i' index could reach 2. shader_bench.cc 152
Возможно, раньше массив 'painters' состоял из трех элементов. Теперь их только два. А значение константы 'kNumPainters' осталось равно 3.
Некоторые другие места в тестах, которые, как мне кажется, заслуживают внимания:
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1790
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1800
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1810
V595 The 'browser' pointer was utilized before it was verified against nullptr. Check lines: 5489, 5493. testing_automation_provider.cc 5489
V595 The 'waiting_for_.get()' pointer was utilized before it was verified against nullptr. Check lines: 205, 222. downloads_api_unittest.cc 205
V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 34, 35. plugin_windowed_test.cc 34
V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 16, 20. plugin_window_size_test.cc 16
V595 The 'textfield_view_' pointer was utilized before it was verified against nullptr. Check lines: 182, 191. native_textfield_views_unittest.cc 182
V595 The 'message_loop_' pointer was utilized before it was verified against nullptr. Check lines: 53, 55. test_flash_message_loop.cc 53
Замеченное N13 — Функция с переменным количеством аргументов
Во всех программах, много дефектов встречается в коде, предназначенного для обработки ошибок и реакции на некорректные входные данные. Это обусловлено тем, что такие места сложно тестировать. И, как правило, они и не тестируются. В результате, при возникновении ошибки, программы начинают вести себя намного более причудливо, чем планировалось.
Пример:
DWORD GetLastError(VOID);
void TryOpenFile(wchar_t *path, FILE *output) {
wchar_t path_expanded[MAX_PATH] = {0};
DWORD size = ::ExpandEnvironmentStrings(
path, path_expanded, MAX_PATH - 1);
if (!size) {
fprintf(output,
"[ERROR] Cannot expand "%S". Error %S.rn",
path, ::GetLastError());
}
....
}V576 Incorrect format. Consider checking the fourth actual argument of the 'fprintf' function. The pointer to string of wchar_t type symbols is expected. fs.cc 17
Если переменная 'size' равна нулю, программа попытается записать в файл текстовое сообщение. Но это сообщение, скорее всего, будет содержать в конце биллиберду. Более того, этот код может привести к access violation.
Для записи использует функция fprintf(). Это функция не контролирует типы своих аргументов. Она ожидает, что последним аргументом должен быть указатель на строку. Но на самом деле, фактическим аргументом является число (код ошибки). Это число будет преобразован в адрес и как дальше поведёт себя программа, неизвестно.
Незамеченное
Ещё раз повторю, что я просматривал список сообщений поверхностно. Я привел в этой статье только то, что привлекло моё внимание. Более того, заметил я больше, чем написал в статье. Описывая всё, я получу слишком длинную статью. А она и так уже великовата.
Я отбросил очень многие фрагменты кода, которые как мне кажется, будут не очень интересны читателю. Приведу пару примеров для ясности.
bool ManagedUserService::UserMayLoad(
const extensions::Extension* extension,
string16* error) const
{
if (extension_service &&
extension_service->GetInstalledExtension(extension->id()))
return true;
if (extension) {
bool was_installed_by_default =
extension->was_installed_by_default();
.....
}
}V595 The 'extension' pointer was utilized before it was verified against nullptr. Check lines: 277, 280. managed_user_service.cc 277
В начале указатель 'extension' разыменовывается в выражении «extension->id()». Затем этот указатель проверяют на равенство нулю.
Часто, подобный код не содержит ошибок. Бывает, что указатель просто не может быть равен нулю и проверка избыточна. Поэтому перечислять такие места нет смысла, так как я могу ошибиться и выдать вполне рабочий код за ошибочный.
Вот ещё пример диагностики, которую я предпочёл не заметить:
bool WebMClusterParser::ParseBlock(....)
{
int timecode = buf[1] << 8 | buf[2];
....
if (timecode & 0x8000)
timecode |= (-1 << 16);
....
}V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. webm_cluster_parser.cc 217
Формально сдвиг отрицательного значения приводит к неопределённому поведению. Однако, многие компиляторы ведут стабильно и именно так, как ожидает программист. В результате этот код долго и успешно работает, хотя не обязан. Мне не хочется сейчас сражаться с этим, и я пропущу подобные сообщения. Для тех, кто хочет подробнее разобраться в вопросе, рекомендую статью "Не зная брода, не лезь в воду — часть третья".
О ложных срабатываниях
Мне часто задают вопрос:
В статьях вы очень ловко приводите примеры найденных ошибок. Но вы не говорите, каково общее количество выдаваемых сообщений. Часто статические анализаторы выдают очень много ложных срабатываний и среди них практически невозможно отыскать настоящие ошибки. Как дело обстоит с анализатором PVS-Studio?
И я всегда не знаю, что сходу ответить на этот вопрос. У меня есть два противоположных ответа: первый — много, второй — мало. Все зависит, как подойти к рассмотрению выданного списка сообщения. Сейчас на примере Chromium я попробую объяснить суть такой двойственности.
Анализатор PVS-Studio выдал 3582 предупреждений первого уровня (набор правил общего назначения GA). Это очень много. И в большинстве этих сообщений являются ложными. Если подойти «в лоб» и начать сразу просматривать весь список, то это очень быстро надоест. И впечатление будет ужасное. Одни сплошные однотипные ложные срабатывания. Ничего интересного не попадается. Плохой инструмент.
Ошибка такого пользователя в том, что не выполнена даже минимальная настройка инструмента. Да, мы стараемся сделать инструмент PVS-Studio таким, чтобы он работал сразу после установки. Мы стараемся, чтобы ничего не надо было настраивать. Человек должен просто проверить свой проект и изучить список выданных предупреждений.
Однако так не всегда получается. Так не получилось и с Chromium. Например, огромное количество ложных сообщений возникло из-за макроса 'DVLOG'. Этот макрос что-то логирует. Он написан хитро и PVS-Studio ошибочно посчитал, что в нем может быть ошибка. Поскольку, макрос очень активно используется, ложных сообщений получилось очень много. Можно сказать, что сколько раз используется макрос DVLOG, столько ложных предупреждений попадет в отчет. А именно, о макросе было выдано около 2300 ложных сообщений «V501 There are identical sub-expressions.....».
Можно подавить эти предупреждения, вписав в заголовочный файл, рядом с объявлением макроса, вот такой вот комментарий:
//-V:DVLOG:501
Смотрите, таким простым действием мы вычтем из 3582 сообщений, 2300 ложных. Мы сразу отсеяли 65% сообщений. И теперь нам не надо их зря просматривать.
Без особых усилий можно сделать ещё несколько подобных уточнений и настроек. В результате, большинство ложных срабатываний исчезнут. В некоторых случаях после настройки следует перезапускать анализ, в некоторых нет. Подробнее всё это описывает в документации раздел "Подавление ложных предупреждений". Конкретно, в случае с ошибками в макросах, придется перезапускать анализ.
Надеюсь теперь понятно. Почему первый ответ — ложных срабатываний много. И почему второй ответ — ложных срабатываний мало. Всё зависит, готов ли человек потратить хоть немного времени на изучение продукта и способов избавить себя от лишних сообщений.
Напутствие читателям
Пользуясь случаем, хочу передать привет родителям. Ой нет, это что-то не то. Пользуясь случаем, хочу передать привет программистам и напомнить, что:
- Ответ на вопрос «Вы сообщили о найденных в проекте ошибках разработчикам?», находится в заметке "Ответы на вопросы, которые часто задают после прочтения наших статей".
- С нами лучше всего связаться и задать вопросы, используя форму обратной связи на нашем сайте. Прошу не использовать для этого twitter, комментарии к статьям где-то на сторонних сайтах и так далее.
- Приглашаю присоединиться к нашему твиттеру: @Code_Analysis. Я постоянно собираю и публикую интересные ссылки по тематике программирования и языку Си++.
Автор: Andrey2008
