Святослав Размыслов

27 Сен 2019

Теги:

#Cpp

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

27 Сен 2019

Автор: Святослав Размыслов

Введение
comm
netwerk
gfx
dom
xpcom
nsprpub
Заключение

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

Введение

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

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

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

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

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

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

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

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

comm

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

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

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 дважды. Возможно, на её месте должна была быть другая константа.

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

static int MimeMultipartSigned_emit_child(MimeObject *obj) {
  ....
  if (obj->options && obj->options->headers != MimeHeadersCitation &&
      obj->options->write_html_p && obj->options->output_fn &&
      obj->options->headers != MimeHeadersCitation && sig->crypto_closure) {
    ....
  }
  ....
}

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

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

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. Как результат - одна из веток никогда не выполняется.

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

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()));
  }
  ....
}

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

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

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 и т.п.

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

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."

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

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.

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 [] mData;'. Check lines: 233, 222. 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.

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

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.

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

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.

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

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.

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

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

V647 The value of 'int' type is assigned to the pointer of 'short' type. Consider inspecting the assignment: 'out_flags = 0x2'. 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

V597 The compiler could delete the 'memset' function call, which is used to flush 'newdeskey' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. 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

V519 The 'state[state_length - x_length + i]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 83, 84. filter_ar.c 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 на своём проекте.

#Cpp