Хабрахабр

Из-за тёмной темы Thunderbird пришлось запускать анализатор кода

Picture 3

«Приключение» с почтовым клиентом Mozilla Thunderbird началось с автоматического обновления на версию 68.0. Заметными особенностями этой версии было вот что: больше текста добавляется во всплывающие уведомления и тёмная тема по умолчанию. Повстречалась ошибка, которую захотелось попробовать обнаружить с помощью статического анализа. Это стало поводом в очередной раз проверить исходный код проекта с помощью PVS-Studio. Так вышло, что к моменту анализа ошибка уже была исправлена. Но раз мы обратили внимание на этот проект, мы можем написать про другие найденные в нём дефекты.

Введение

Тёмная тема новой версии Thunderbird выглядит достаточно красиво. Я люблю тёмные темы. Уже перешёл на них в мессенджерах, Windows, macOS. Скоро iPhone обновится до iOS 13, где появилась тёмная тема. Ради этого даже пришлось сменить свой iPhone 5S на более новую модель. На практике оказалось, что тёмная тема требует больше усилий для разработчиков, чтобы подобрать цвета интерфейса. Не все с этим справляются с первого раза.Так у меня стали выглядеть стандартные теги в Thunderbird:

Picture 1

На половину из них после обновления стало невозможно смотреть, и я решил в настройках изменить цвет на более яркий. Я пользуюсь шестью тегами (5 стандартных + 1 пользовательский) для разметки писем. Но тут я столкнулся с багом:

Picture 2

Точнее, можно, но редактор не даст его сохранить, ссылаясь на уже существующее имя (WTF???). Нельзя поменять цвет тега!!!

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

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

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

Исходный код Thunderbird так или иначе пересекается с кодовой базой Firefox. Примечание. Поэтому в анализ вошли ошибки из разных компонентов, которые стоит внимательно посмотреть разработчикам разных команд.

Пока писалась статья, вышло обновление Thunderbird 68. Примечание 2. 1 с исправлением этого бага:

Picture 5

comm

comm-central is a Mercurial repository of the Thunderbird, SeaMonkey, and Lightning extension code.

nsEmitterUtils.cpp 28 V501 There are identical sub-expressions '(!strcmp(header, «Reply-To»))' to the left and to the right of the '||' operator.

extern "C" bool EmitThisHeaderForPrefSetting(int32_t dispType, const char *header) { .... if (nsMimeHeaderDisplayTypes::NormalHeaders == dispType) { if ((!strcmp(header, HEADER_DATE)) || (!strcmp(header, HEADER_TO)) || (!strcmp(header, HEADER_SUBJECT)) || (!strcmp(header, HEADER_SENDER)) || (!strcmp(header, HEADER_RESENT_TO)) || (!strcmp(header, HEADER_RESENT_SENDER)) || (!strcmp(header, HEADER_RESENT_FROM)) || (!strcmp(header, HEADER_RESENT_CC)) || (!strcmp(header, HEADER_REPLY_TO)) || (!strcmp(header, HEADER_REFERENCES)) || (!strcmp(header, HEADER_NEWSGROUPS)) || (!strcmp(header, HEADER_MESSAGE_ID)) || (!strcmp(header, HEADER_FROM)) || (!strcmp(header, HEADER_FOLLOWUP_TO)) || (!strcmp(header, HEADER_CC)) || (!strcmp(header, HEADER_ORGANIZATION)) || (!strcmp(header, HEADER_REPLY_TO)) || (!strcmp(header, HEADER_BCC))) return true; else return false; ....
}

Строку header сравнили с константой HEADER_REPLY_TO дважды. Возможно, на её месте должна была быть другая константа.

mimemsig.cpp 536 V501 There are identical sub-expressions 'obj->options->headers != MimeHeadersCitation' to the left and to the right of the '&&' operator.

static int MimeMultipartSigned_emit_child(MimeObject *obj) ....
}

Ещё одно странное сравнение переменной с похожим именем — headers. Как всегда, есть два возможных объяснения: лишняя проверка или опечатка.

There is a probability of logical error presence. V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. MapiApi.cpp 1306 Check lines: 1306, 1308.

void CMapiApi::ReportLongProp(const char *pTag, LPSPropValue pVal) { if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_LONG)) { nsCString num; nsCString num2; num.AppendInt((int32_t)pVal->Value.l); num2.AppendInt((int32_t)pVal->Value.l, 16); MAPI_TRACE3("%s %s, 0x%s\n", pTag, num, num2); } else if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_NULL)) { MAPI_TRACE1("%s {NULL}\n", pTag); } else if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_ERROR)) { // <= MAPI_TRACE1("%s {Error retrieving property}\n", pTag); } else if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_ERROR)) { // <= MAPI_TRACE1("%s {Error retrieving property}\n", pTag); } else { MAPI_TRACE1("%s invalid value, expecting long\n", pTag); } if (pVal) MAPIFreeBuffer(pVal);
}

Написание каскада условных выражений явно было ускорено клавишами Ctrl+C и Ctrl+V. Как результат — одна из веток никогда не выполняется.

There is a probability of logical error presence. V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. nsRDFContentSink.cpp 777 Check lines: 777, 816.

nsresult
RDFContentSinkImpl::GetIdAboutAttribute(const char16_t** aAttributes, nsIRDFResource** aResource, bool* aIsAnonymous)
{ .... if (localName == nsGkAtoms::about) { .... } else if (localName == nsGkAtoms::ID) { .... } else if (localName == nsGkAtoms::nodeID) { nodeID.Assign(aAttributes[1]); } else if (localName == nsGkAtoms::about) { // XXX we don't deal with aboutEach... //MOZ_LOG(gLog, LogLevel::Warning, // ("rdfxml: ignoring aboutEach at line %d", // aNode.GetSourceLineNumber())); } ....
}

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

morkRowCellCursor.cpp 175 V522 Dereferencing of the null pointer 'row' might take place.

NS_IMETHODIMP
morkRowCellCursor::MakeCell( // get cell at current pos in the row nsIMdbEnv* mev, // context mdb_column* outColumn, // column for this particular cell mdb_pos* outPos, // position of cell in row sequence nsIMdbCell** acqCell) { nsresult outErr = NS_OK; nsIMdbCell* outCell = 0; mdb_pos pos = 0; mdb_column col = 0; morkRow* row = 0; morkEnv* ev = morkEnv::FromMdbEnv(mev); if (ev) { pos = mCursor_Pos; morkCell* cell = row->CellAt(ev, pos); if (cell) { col = cell->GetColumn(); outCell = row->AcquireCellHandle(ev, cell, col, pos); } outErr = ev->AsErr(); } if (acqCell) *acqCell = outCell; if (outPos) *outPos = pos; if (outColumn) *outColumn = col; return outErr;
}

Разыменование нулевого указателя row возможно в следующей строке:

morkCell* cell = row->CellAt(ev, pos);

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

MapiApi.cpp 1050 V543 It is odd that value '-1' is assigned to the variable 'm_lastError' of HRESULT type.

class CMapiApi { .... private: static HRESULT m_lastError; ....
}; CMsgStore *CMapiApi::FindMessageStore(ULONG cbEid, LPENTRYID lpEid) { if (!m_lpSession) { MAPI_TRACE0("FindMessageStore called before session is open\n"); m_lastError = -1; return NULL; } ....
}

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

Ещё пара таких мест:

  • V543 It is odd that value '-1' is assigned to the variable 'm_lastError' of HRESULT type. MapiApi.cpp 817
  • V543 It is odd that value '-1' is assigned to the variable 'm_lastError' of HRESULT type. MapiApi.cpp 1749

V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. icalmime.c 195

icalcomponent* icalmime_parse(....)
{ struct sspm_part *parts; int i, last_level=0; icalcomponent *root=0, *parent=0, *comp=0, *last = 0; if ( (parts = (struct sspm_part *) malloc(NUM_PARTS*sizeof(struct sspm_part)))==0) { icalerror_set_errno(ICAL_NEWFAILED_ERROR); return 0; } memset(parts,0,sizeof(parts)); sspm_parse_mime(parts, NUM_PARTS, /* Max parts */ icalmime_local_action_map, /* Actions */ get_string, data, /* data for get_string*/ 0 /* First header */); ....
}

Переменная parts является указателем на массив структур. Для сброса значений структур воспользовались функцией memset, но в качестве размера участка памяти ей передали размер указателя.

Другие подозрительные места:

  • V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. icalmime.c 385
  • V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. icalparameter.c 114
  • V579 The snprintf function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. icaltimezone.c 1908
  • V579 The snprintf function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. icaltimezone.c 1910
  • V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. sspm.c 707
  • V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. sspm.c 813

V595 The 'aValues' pointer was utilized before it was verified against nullptr. Check lines: 553, 555. nsLDAPMessage.cpp 553

NS_IMETHODIMP
nsLDAPMessage::GetBinaryValues(const char *aAttr, uint32_t *aCount, nsILDAPBERValue ***aValues) { .... *aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; } ....
}

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

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

*aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue)));
if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY;
}

Ещё одно очень похожее место:

  • V595 The '_retval' pointer was utilized before it was verified against nullptr. Check lines: 357, 358. nsLDAPSyncQuery.cpp 357

V1044 Loop break conditions do not depend on the number of iterations. mimemoz2.cpp 1795

void ResetChannelCharset(MimeObject *obj) { .... if (cSet) { char *ptr2 = cSet; while ((*cSet) && (*cSet != ' ') && (*cSet != ';') && (*cSet != '\r') && (*cSet != '\n') && (*cSet != '"')) ptr2++; if (*cSet) { PR_FREEIF(obj->options->default_charset); obj->options->default_charset = strdup(cSet); obj->options->override_charset = true; } PR_FREEIF(cSet); } ....
}

Эта ошибка найдена с помощью новой диагностики, которая будет доступна в следующем релизе анализатора. Все переменные, используемые в условии остановки цикла while, не модифицируются, потому что в теле функции перепутали переменные ptr2 и cSet.

netwerk

netwerk contains C interfaces and code for low-level access to the network (using sockets and file and memory caches) as well as higher-level access (using various protocols such as http, ftp, gopher, castanet). This code is also known by the names, «netlib» and «Necko.»

nsSocketTransport2.cpp 1693 V501 There are identical sub-expressions 'connectStarted' to the left and to the right of the '&&' operator.

nsresult nsSocketTransport::InitiateSocket() { .... if (gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase() && connectStarted && connectCalled) { // <= good, line 1630 SendPRBlockingTelemetry( connectStarted, Telemetry::PRCONNECT_BLOCKING_TIME_NORMAL, Telemetry::PRCONNECT_BLOCKING_TIME_SHUTDOWN, Telemetry::PRCONNECT_BLOCKING_TIME_CONNECTIVITY_CHANGE, Telemetry::PRCONNECT_BLOCKING_TIME_LINK_CHANGE, Telemetry::PRCONNECT_BLOCKING_TIME_OFFLINE); } .... if (gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase() && connectStarted && connectStarted) { // <= fail, line 1694 SendPRBlockingTelemetry( connectStarted, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_NORMAL, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_SHUTDOWN, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_CONNECTIVITY_CHANGE, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_LINK_CHANGE, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_OFFLINE); } ....
}

Я сначала подумал, что дублирование переменной connectStarted — это просто лишний код, пока не просмотрел всю достаточно длинную функцию и не обнаружил похожий фрагмент. Скорее всего, вместо одной переменной connectStarted тут тоже должна быть переменная connectCalled.

Consider inspecting this code. V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Check lines: 233, 222. It's probably better to use 'delete [] mData;'. DataChannel.cpp 233

BufferedOutgoingMsg::BufferedOutgoingMsg(OutgoingMsg& msg) { size_t length = msg.GetLeft(); auto* tmp = new uint8_t[length]; // infallible malloc! memcpy(tmp, msg.GetData(), length); mLength = length; mData = tmp; mInfo = new sctp_sendv_spa; *mInfo = msg.GetInfo(); mPos = 0;
} BufferedOutgoingMsg::~BufferedOutgoingMsg() { delete mInfo; delete mData;
}

Указатель mData указывает на массив, а не на один объект. В деструкторе класса допустили ошибку, забыв добавить квадратные скобки для оператора delete.

ParseFTPList.cpp 691 V1044 Loop break conditions do not depend on the number of iterations.

int ParseFTPList(....) { .... pos = toklen[2]; while (pos > (sizeof(result->fe_size) - 1)) pos = (sizeof(result->fe_size) - 1); memcpy(result->fe_size, tokens[2], pos); result->fe_size[pos] = '\0'; ....
}

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

gfx

gfx contains C interfaces and code for platform independent drawing and imaging. It can be used to draw rectangles, lines, images, etc. Essentially, it is a a set of interfaces for a platform-independent device (drawing) context. It does not handle widgets or specific drawing routines; it just provides the primitive operations for drawing.

V501 There are identical sub-expressions to the left and to the right of the '||' operator: mVRSystem || mVRCompositor || mVRSystem OpenVRSession.cpp 876

void OpenVRSession::Shutdown() { StopHapticTimer(); StopHapticThread(); if (mVRSystem || mVRCompositor || mVRSystem) { ::vr::VR_Shutdown(); mVRCompositor = nullptr; mVRChaperone = nullptr; mVRSystem = nullptr; }
}

В условии два раза присутствует переменная mVRSystem. Очевидно, одну из них следует заменить на mVRChaperone.

dom

dom contains C interfaces and code for implementing and tracking DOM (Document Object Model) objects in Javascript. It forms the C substructure which creates, destroys and manipulates built-in and user-defined objects according to the Javascript script.

Document.cpp 12049 V570 The 'clonedDoc->mPreloadReferrerInfo' variable is assigned to itself.

already_AddRefed<Document> Document::CreateStaticClone( nsIDocShell* aCloneContainer) { .... clonedDoc->mReferrerInfo = static_cast<dom::ReferrerInfo*>(mReferrerInfo.get())->Clone(); clonedDoc->mPreloadReferrerInfo = clonedDoc->mPreloadReferrerInfo; ....
}

Анализатор обнаружил присваивание переменной самой себе.

xpcom

xpcom contains the low-level C interfaces, C code, C code, a bit of assembly code and command line tools for implementing the basic machinery of XPCOM components (which stands for «Cross Platform Component Object Model»). XPCOM is the mechanism that allows Mozilla to export interfaces and have them automatically available to JavaScript scripts, to Microsoft COM and to regular Mozilla C code.

Consider inspecting operation logics behind the 'key' variable. V611 The memory was allocated using 'malloc/realloc' function but was released using the 'delete' operator. nsINIParser.h 143 Check lines: 143, 140.

struct INIValue { INIValue(const char* aKey, const char* aValue) : key(strdup(aKey)), value(strdup(aValue)) {} ~INIValue() { delete key; delete value; } void SetValue(const char* aValue) { delete value; value = strdup(aValue); } const char* key; const char* value; mozilla::UniquePtr<INIValue> next;
};

После вызова функции strdup необходимо освобождать память с помощью функции free, а не оператора delete.

SpecialSystemDirectory.cpp 73 V716 Suspicious type conversion in initialization: 'HRESULT var = BOOL'.

BOOL SHGetSpecialFolderPathW( HWND hwnd, LPWSTR pszPath, int csidl, BOOL fCreate
); static nsresult GetWindowsFolder(int aFolder, nsIFile** aFile) { WCHAR path_orig[MAX_PATH + 3]; WCHAR* path = path_orig + 1; HRESULT result = SHGetSpecialFolderPathW(nullptr, path, aFolder, true); if (!SUCCEEDED(result)) { return NS_ERROR_FAILURE; } ....
}

WinAPI функция SHGetSpecialFolderPathW возвращает значение типа BOOL, а не HRESULT. Проверку результата функции необходимо переписать на правильную.

nsprpub

nsprpub contains C code for the cross platform «C» Runtime Library. The «C» Runtime Library contains basic non-visual C functions to allocate and deallocate memory, get the time and date, read and write files, handle threads and handling and compare strings across all platforms

Consider inspecting the assignment: 'out_flags = 0x2'. V647 The value of 'int' type is assigned to the pointer of 'short' type. prsocket.c 1220

#define PR_POLL_WRITE 0x2 static PRInt16 PR_CALLBACK SocketPoll( PRFileDesc *fd, PRInt16 in_flags, PRInt16 *out_flags)
{ *out_flags = 0; #if defined(_WIN64) if (in_flags & PR_POLL_WRITE) { if (fd->secret->alreadyConnected) { out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; } }
#endif return in_flags;
} /* SocketPoll */

Анализатор обнаружил присваивание численной константы указателю out_flags. Скорее всего, его просто забыли разыменовать:

if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE;
}

Заключение

Это ещё не конец. Новым обзорам кода быть. В состав кода Thunderbird и Firefox входят две крупные библиотеки: Network Security Services (NSS) и WebRTC (Web Real Time Communications). Там нашлись очень интересные ошибки. В этом обзоре покажу по одной.

NSS

The RtlSecureZeroMemory() function should be used to erase the private data. V597 The compiler could delete the 'memset' function call, which is used to flush 'newdeskey' buffer. pkcs11c.c 1033

static CK_RV
sftk_CryptInit(....)
{ .... unsigned char newdeskey[24]; .... context->cipherInfo = DES_CreateContext( useNewKey ? newdeskey : (unsigned char *)att->attrib.pValue, (unsigned char *)pMechanism->pParameter, t, isEncrypt); if (useNewKey) memset(newdeskey, 0, sizeof newdeskey); sftk_FreeAttribute(att); ....
}

NSS — это библиотека для разработки защищенных клиентских и серверных приложений, а тут DES Key не чистится. Компилятор удалит вызов memset из кода, т.к. массив newdeskey больше не используется в коде дальше этого места.

WebRTC

Perhaps this is a mistake. V519 The 'state[state_length — x_length + i]' variable is assigned values twice successively. filter_ar.c 84 Check lines: 83, 84.

size_t WebRtcSpl_FilterAR(....)
{ .... for (i = 0; i < state_length - x_length; i++) { state[i] = state[i + x_length]; state_low[i] = state_low[i + x_length]; } for (i = 0; i < x_length; i++) { state[state_length - x_length + i] = filtered[i]; state[state_length - x_length + i] = filtered_low[i]; // <= } ....
}

Во втором цикле данные записываются не в тот массив, потому что автор скопировал код и забыл изменить название массива со state на state_low.

И мы этим займёмся в ближайшее время. Вероятно, в этих проектах есть ещё интересные ошибки, о которых стоит рассказать. А пока попробуйте PVS-Studio на своём проекте.

Dark theme of Thunderbird as a reason to run a code analyzer. Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov.

Показать больше

Похожие публикации

Добавить комментарий

Ваш адрес email не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»