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

Введение

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

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

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

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

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

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

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

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

связь

comm-central — это репозиторий Mercurial кода расширения Thunderbird, SeaMonkey и Lightning.

V501 Слева и справа от оператора || одинаковые подвыражения ‘(!strcmp(header, Reply-To))’. 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_REPLY_TO . Возможно, на его месте должна была быть другая константа.

V501 Слева и справа от оператора && одинаковые подвыражения obj-›options-›headers != MimeHeadersCitation. 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 Обнаружено использование шаблона if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Строки проверки: 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 Обнаружено использование шаблона if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Проверить строки: 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 Может иметь место разыменование строки нулевого указателя. 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 Странно, что переменной m_lastError типа HRESULT присвоено значение -1. 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 Странно, что переменной m_lastError типа HRESULT присвоено значение «-1». MapiApi.cpp 817
  • V543 Странно, что переменной m_lastError типа HRESULT присвоено значение «-1». MapiApi.cpp 1749

V579 Функция memset получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте третий аргумент. 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 Функция memset получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте третий аргумент. 385
  • V579 Функция memset получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте третий аргумент. icalparameter.c 114
  • V579 Функция snprintf получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте второй аргумент. icaltimezone.c 1908 г.
  • V579 Функция snprintf получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте второй аргумент. icaltimezone.c 1910
  • V579 Функция strncmp получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте третий аргумент. sspm.c 707
  • V579 Функция strncmp получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте третий аргумент. sspm.c 813

V595 Указатель aValues использовался до того, как он был проверен на соответствие nullptr. Проверьте строки: 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 Указатель _retval использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 357, 358. nsLDAPSyncQuery.cpp 357

V1044 Условия выхода из цикла не зависят от количества итераций. 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 содержит интерфейсы C и код для низкоуровневого доступа к сети (используя сокеты и кэши файлов и памяти), а также для доступа более высокого уровня (используя различные протоколы, такие как http, ftp, gopher, castanet). Этот код также известен под именами «netlib» и «Necko».

V501 Слева и справа от оператора && одинаковые подвыражения connectStarted. 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 — это просто лишний код. Но потом я просмотрел всю довольно длинную функцию и нашел похожий фрагмент. Скорее всего, здесь должна быть переменная connectCalled вместо переменной connectStarted.

V611 Память была выделена с помощью оператора new T[], но освобождена с помощью оператора удалить. Рассмотрите возможность проверки этого кода. Вероятно, лучше использовать удалить [] mData;. Проверить строки: 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 Условия выхода из цикла не зависят от количества итераций. 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 содержит интерфейсы C и код для независимого от платформы рисования и визуализации. Его можно использовать для рисования прямоугольников, линий, изображений и т. д. По сути, это набор интерфейсов для независимого от платформы контекста устройства (рисования). Он не обрабатывает виджеты или специальные процедуры рисования; он просто предоставляет примитивные операции для рисования.

V501 Слева и справа от оператора || находятся одинаковые подвыражения: mVRSystem || mVRКомпозитор || 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 содержит интерфейсы C и код для реализации и отслеживания объектов DOM (объектная модель документа) в Javascript. Он формирует подструктуру C, которая создает, уничтожает и манипулирует встроенными и определяемыми пользователем объектами в соответствии со сценарием Javascript.

V570 Переменная clonedDoc-›mPreloadReferrerInfo присваивается самой себе. Документ.cpp 12049

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

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

хпком

xpcom содержит низкоуровневые интерфейсы C, код C, код C, немного ассемблерного кода и инструменты командной строки для реализации основных механизмов компонентов XPCOM (что означает «Кросс-платформенная объектная модель компонентов»). XPCOM — это механизм, который позволяет Mozilla экспортировать интерфейсы и делать их автоматически доступными для сценариев JavaScript, Microsoft COM и обычного кода Mozilla C.

V611 Память была выделена с помощью функции malloc/realloc, но освобождена с помощью оператора удалить. Рассмотрите возможность проверки логики работы за переменной ключ. Проверить строки: 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 Подозрительное преобразование типа при инициализации: ‘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;
  }
  ....
}

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

нспрпаб

nsprpub содержит код C для межплатформенной библиотеки времени выполнения «C». Библиотека времени выполнения «C» содержит основные невизуальные функции C для выделения и освобождения памяти, получения времени и даты, чтения и записи файлов, обработки потоков, обработки и сравнения строк на всех платформах.

V647 Указатель типа короткий присваивается значение типа int. Рассмотрите возможность проверки назначения: 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). Я нашел там несколько убедительных ошибок. В этом обзоре я покажу по одному из каждого проекта.

НСС

V597 Компилятор мог удалить вызов функции memset, которая используется для сброса буфера newdeskey. Функцию RtlSecureZeroMemory() следует использовать для стирания личных данных. 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 здесь не очищается. Компилятор удалит вызов memset из кода, так как массив newdeskey больше нигде в коде не используется.

Веб-RTC

V519 Переменной state[state_length — x_length + i] дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 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 на своем проекте.