Мы используем куки, чтобы пользоваться сайтом
было удобно.
Если честно, я не знаю, для чего предназначен проект TPP. Как мне кажется, это набор инструментов для изучения белков и их взаимодействия в живых организмах. Впрочем, это не так важно. Главное, что исходные коды открыты. Значит, я могу проверить их с помощью статического анализатора PVS-Studio. А я люблю это делать.
Итак, был проверен проект Trans-Proteomic Pipeline (TPP) версии 4.5.2. Подробнее о проекте можно узнать по следующим ссылкам:
Мы пишем не обо всех проверенных проектах. Проект должен чем-то выделяться. В противном случае, описание найденных ошибок будет скучным. Например, проект должен быть известным, или содержать много ошибок определенного рода, или что-то ещё. Проект TPP обратил на себя внимание разнообразностью ошибок. Как правило, в проектах преобладают те или иные виды дефектов. В случае TPP мало повторяющихся ошибок. Они разнообразны и это интересно.
Следует отметить, что многие ошибки относятся не к самому проекту TPP, а к используемой в нём библиотеке для работы с XML. Однако я не думаю, что есть разница, будет неправильно обработан файл по вине программы или библиотеки XML. С точки зрения пользователя ошибка будет относиться к библиотеке TPP. Поэтому и я не буду уточнять, к какой части проекта относится тот или иной дефект. И хватит мне болтать. Давайте скорее преступим к интересным примерам.
К сожалению, я не знаю, что такое пептиды. Википедия подсказывает мне, что это семейство веществ, молекулы которых построены из остатков a-аминокислот, соединённых в цепь пептидными (амидными) связями -C(O)NH-. Вполне ожидаемо, что в TPP есть класс, названный Peptide, а в нём оператор сравнения. Реализован он следующим образом:
bool Peptide::operator==(Peptide& p) {
...
for (i = 0, j = 0;
i < this->stripped.length(), j < p.stripped.length();
i++, j++) {
...
}PVS-Studio: V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. tpplib peptide.cpp 191
Обратите внимание, что два сравнения записаны через оператор запятая ','. Оператор запятая возвращает значение выражения, расположенное справа. Это значит, что проверяется только одно условие: "j < p.stripped.length()". Корректное сравнение должно выглядеть следующим образом:
for (i = 0, j = 0;
i < this->stripped.length() && j < p.stripped.length();
i++, j++) {Аналогичная ошибка допущена в методе Peptide::strippedEquals(). Я беспокоюсь за пептиды :).
Когда в программе работают с файловыми путями, часто нужно, чтобы путь заканчивался на косую \ или /. Для этого написан следующий код:
bool TandemResultsParser::writePepXML(....)
{
...
char c = pathSummary.at(pathSummary.length() - 1);
if (c != '\\' || c != '/')
{
if (pathSummary.find('\\') != string::npos)
pathSummary.append("\\");
else
pathSummary.append("/");
}
...
}PVS-Studio: V547 Expression 'c != '\\' || c != '/'' is always true. Probably the '&&' operator should be used here. Tandem2XML tandemresultsparser.cxx 787
Если внимательно посмотреть на условие if (c != '\\' || c != '/'), то можно заметить опечатку. Условие всегда истинно. Переменная 'c' будет не равна '\\' или не равна '/'. В результате, в конце пути может появиться две косых. Возможно, эта ошибка не критична, но неприятна.
Корректное условие:
if (c != '\\' && c != '/')Рассмотрим код, который должен найти в строке подстроку " PI ":
class basic_string
{
...
size_type find(const _Elem *_Ptr, size_type _Off = 0) const
...
}
void PipelineAnalysis::prepareFields(void) {
...
if (peptideProphetOpts_.find(" PI ", 0)>=0) {
fields_.push_back(Field("PpI_zscore"));
}
...
}PVS-Studio: V547 Expression 'peptideProphetOpts_.find(" PI ", 0) >= 0' is always true. Unsigned type value is always >= 0. pepXMLViewer pipelineanalysis.cxx 1590
Ошибочно используется функция std::string::find(). Если подстрока не находится, функция find() возвращает значение string::npos. Причем, это значение имеет беззнаковый тип.
В программе же предполагается, что если подстрока не найдена, функция find() должна вернуть отрицательное число. Это никогда не произойдёт. Условие "peptideProphetOpts_.find(" PI ", 0)>=0" всегда истинно, ведь беззнаковое значение всегда >= 0.
В результате, в независимости от того что содержит строка 'peptideProphetOpts', мы всегда пометим её как "PpI_zscore". И ещё, аналогичная ошибка находится в этой же функции чуть ниже (строка 1593). Я вновь волнуюсь за пептиды.
Корректный поиск подстроки должен выглядеть так:
if (peptideProphetOpts_.find(" PI ", 0) != string::npos)Фрагмент кода, генерирующий случайные числа может привести к гораздо большему количеству случайностей, чем хочется. Рассмотрим код:
int main(int argc, char **argv) {
...
char salt[3];
...
salt[0] = (argc>2)?(argv[1][0]):rndChar[rand() % 64];
salt[1] = (argc>2)?(argv[1][1]):rndChar[rand() % 64];
salt[3] = 0;
...
}PVS-Studio: V557 Array overrun is possible. The '3' index is pointing beyond array bound. crypt crypt.cxx 567
Код должен заполнить маленький массив двумя случайными числами и нулём. Но ноль записывается мимо массива. В результате, последний элемент массива остаётся неинициализированным. Также, происходит запись значения за пределы массива, что тоже неприятно. Всё это вызывает неопределенное поведение программы в дальнейшем.
Исправленный код:
salt[2] = 0;void DIGEST_PROTEIN(char *szSeq, int iLenSeq)
{
...
if (pOptions.bMarkNXST
&& szPeptide[i] == 'N'
&& szPeptide[i + 1] != 'P'
&& (szPeptide[i + 2] == 'S' ||
szPeptide[i + 2] == 'T')
&& szPeptide[i + 1] != 'P')
...
}PVS-Studio: V501 There are identical sub-expressions 'szPeptide[i + 1] != 'P'' to the left and to the right of the '&&' operator. Comet_fastadb comet-fastadb1.cxx 1906
Ячейка массива 'szPeptide[i + 1]' два раза сравнивается с 'P'. В результате тип пептида проверяется только приблизительно. Я думаю, в последней строке имеет место опечатка и код должен выглядеть так:
if (pOptions.bMarkNXST
&& szPeptide[i] == 'N'
&& szPeptide[i + 1] != 'P'
&& (szPeptide[i + 2] == 'S' ||
szPeptide[i + 2] == 'T')
&& szPeptide[i + 3] != 'P')Строчки в программе длинные, поэтому я заменил кое-что на многоточие. Не обращайте на это внимание. Ничего интересного я не удалил.
void MascotConverter::init(....) {
...
if(line_len > 8 && .... && line[7] == '=')
if(database_ == NULL)
database_ = strCopy(line+8);
else if(line_len > 5 && .... && line[4] == '=') {
header_ = strCopy(line+5);
...
}Посмотрите на 'else if'. Видите подвох? Оператор else относится ко второму, а не к первому оператору 'if'. Если правильно отформатировать код, он бы стал выглядеть так:
if(line_len > 8 && .... && line[7] == '=')
if(database_ == NULL)
database_ = strCopy(line+8);
else if(line_len > 5 && .... && line[4] == '=') {
header_ = strCopy(line+5);
...Странная логика? Согласен. Скорее всего здесь логическая ошибка, а не ошибка оформления кода. Видимо, правильный код должен выглядеть так:
if(line_len > 8 && .... && line[7] == '=')
{
if(database_ == NULL)
database_ = strCopy(line+8);
}
else if(line_len > 5 && .... && line[4] == '=') {
header_ = strCopy(line+5);
...Вывод: не стоит жадничать и экономить место на фигурных скобках.
Чтобы не дублировать код, всегда есть соблазн позвать один конструктор из другого конструктора. К сожалению, при этом легко допустить ошибку. Вот как выглядит эта ошибка:
class ExperimentCycleRecord {
public:
ExperimentCycleRecord()
{ ExperimentCycleRecord(0,0,0,True,False); }
...
}PVS-Studio: V603 The object was created but it is not being used. If you wish to call constructor, 'this->ExperimentCycleRecord::ExperimentCycleRecord(....)' should be used. Mascot2XML mascotconverter.cxx 101
Высказывание "ExperimentCycleRecord(0,0,0,True,False);" создаёт временный объект и успешно его инициализирует. Но это не инициализирует поля текущего класса. Подробнее данный вид ошибки рассмотрен в статье: Не зная брода, не лезь в воду. Часть первая. В этой же статье предлагаются варианты, как исправить такие ошибки.
Аналогичные ошибки можно встретить ещё в некоторых местах:
void TRANSLATE(int iFrame, char *szNewSeq,
char *szSeq, int *iLenSeq)
{
...
*iLenSeq;
}PVS-Studio: V607 Ownerless expression '* iLenSeq'. Comet_fastadb comet-fastadb1.cxx 2241
В конце функции 'TRANSLATE' есть непонятное высказывание "*iLenSeq;". Это высказывание ничего не делает. Возможно, это просто лишняя строчка. А возможно, здесь что-то не дописано. Но это вопрос, что именно...
Вновь пришло время поволноваться о пептидах. Рассмотрим код:
void MixtureModel::assessPeptideProperties(char* filename, Boolean
icat, Boolean glyc)
{
...
double fval;
...
// fval не инициализируется
...
if(! icat && strstr(pep, "C") != NULL && fval >= min_fval) {
...
}PVS-Studio: V614 Uninitialized variable 'fval' used. tpplib mixturemodel.cxx 834
Как поведёт себя проверка - неизвестно. Переменная 'fval' нигде не инициализируется.
А вот цикл, который будет выполняться неизвестное количество раз:
double mscore_c::dot_hr(unsigned long *_v)
{
...
int iSeqSize;
//perform a single pass through each array.
//check every point in m_pfSeq,
//but don't revisit positions in m_vmiType
for (int a = 0; a < iSeqSize; a++) {
...
}PVS-Studio: V614 Uninitialized variable 'iSeqSize' used. xtandem mscore_c.cpp 552
Переменная 'iSeqSize' не инициализирована.
Есть и другие неинициализированные переменные. Подробнее на них я останавливаться не буду и приведу просто их список:
Смотришь на всё это и удивляешься. И опасаешься. Интересные результаты могут выдавать научные исследования, основанные на неинициализированных переменных :).
Следующий фрагмент кода должен считать сумму элементов. Но из-за перепутанных местами двух символов, он этого не делает.
int main(int argc, char **argv)
{
...
ii=0;
for (i=0; pEnvironment.szPeptide[i]!=0; i++)
ii =+ pEnvironment.szPeptide[i];
...
}PVS-Studio: V588 The expression of the 'A =+ B' kind is utilized. Consider reviewing it, as it is possible that 'A += B' was meant. plot_msms plot-msms1.cxx 478
Ошибка элементарна. Однако она не перестаёт от этого быть ошибкой. Этот код хорошо демонстрирует, что многие дефекты в программах просты. Их намного больше, чем думают программисты. Подробнее об этот феномене я писал здесь: "Мифы о статическом анализе. Миф второй – профессиональные разработчики не допускают глупых ошибок".
Корректный код:
for (i=0; pEnvironment.szPeptide[i]!=0; i++)
ii += pEnvironment.szPeptide[i];Рассмотрим реализацию итератора.
CharIndexedVectorIterator& operator++()
{ // preincrement
++m_itr;
return (*this);
}
CharIndexedVectorIterator& operator--()
{ // predecrement
++m_itr;
return (*this);
}PVS-Studio: V524 It is odd that the body of '--' function is fully equivalent to the body of '++' function (charindexedvector.hpp, line 68). pwiz charindexedvector.hpp 81
Оператор '++' написан правильно. А вот оператор '--' видимо писался методом Copy-Paste. В результате, он ведёт себя в точности также, как оператор '++'. Впрочем, другие операторы '--' также написаны подобным образом. Возможно это не ошибка, а хитрая задумка.
Был найден цикл, который выполняется только один раз.
const char* ResidueMass::getStdModResidues(....) {
...
for (rmap::const_iterator i = p.first; i != p.second; ++i) {
const cResidue &r = (*i).second;
if (r.m_masses[0].m_nterm) {
n_term_aa_mod = true;
} else if (r.m_masses[0].m_cterm) {
c_term_aa_mod = true;
}
return r.m_residue.c_str();
}
...
}PVS-Studio: V612 An unconditional 'return' within a loop. tpplib residuemass.cxx 1442
В конце тела цикла имеется оператор 'return'. При этом видно, что в цикле нет оператора 'continue' или иных способов продолжить цикл. Это значит, что цикл выполняется только один раз. Мне сложно сказать, как должен выглядеть этот цикл. Возможно перед оператором 'return' необходимо вписать 'else'.
void ASAPCGIParser::writeProteinRatio(....)
{
...
pvalue = (double)norm_->normalize(adj_inv_ratio);
double tmp[2];
tmp[0] = adj_inv_ratio[0];
tmp[1] = adj_inv_ratio[1];
adj_inv_ratio[0] = 1/ tmp[0];
adj_inv_ratio[1] = tmp[1]/(tmp[0]*tmp[0]);
pvalue = (double)norm_->normalize(adjratio);
...
}PVS-Studio: V519 The 'pvalue' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 205, 214. tpplib asapcgiparser.cxx 214 (...)
Переменная 'pvalue' два раза подряд инициализируется разными значениями. Это подозрительно. Возможно, забыли инициализировать какую-то другую переменную.
Есть еще несколько дефектов, связанных со сравнением беззнаковых переменных с нулём. Например, встретилось ещё одно место, где не заладилась работа с косыми \, /.
int Dta2mzXML::extractScanNum(const string& dtaFileName)
{
...
std::string::size_type pos = dtaFileName.rfind("/");
if (pos < 0) {
pos = dtaFileName.rfind("\\");
}
...
}PVS-Studio: V547 Expression 'pos < 0' is always false. Unsigned type value is never < 0. dta2mzXML dta2mzxml.cpp 622
Переменная 'pos' всегда больше или равна 0. Мы уже рассматривали подобную ситуацию выше. Вот ещё несколько таких ошибок:
С функциями поиска разобрались. Осталось только пара ошибок по этой теме. Первая ошибка:
void SpectraSTReplicates::aggregateStats(....)
{
...
unsigned int numAssignedPeaks =
(*r)->entry->getPeakList()->getNumAssignedPeaks();
if (numAssignedPeaks >= 0) {
sumFracAssigned += (double)numAssignedPeaks/(double)numPeaks;
numAnnotated++;
}
...
}PVS-Studio: V547 Expression 'numAssignedPeaks >= 0' is always true. Unsigned type value is always >= 0. tpplib spectrastreplicates.cpp 642
Думаю, комментарии и разъяснения здесь будут лишними. Вторая ошибка находится здесь:
V547 Expression 'pl->getNumAssignedPeaks() >= 0' is always true. Unsigned type value is always >= 0. tpplib spectrastreplicates.cpp 724
Встретился подозрительный код, где вне зависимости от условия будет исполнен один и тот-же код. Возможно, это последствие Copy-Paste.
bool KernelDensityRTMixtureDistr::recalc_RTstats(....)
{
...
if (catalog) {
tmp = (*run_RT_calc_)[i]->recalc_RTstats(
(*probs)[i], min_prob, (*ntts)[i], min_ntt, 2700);
}
else {
tmp = (*run_RT_calc_)[i]->recalc_RTstats(
(*probs)[i], min_prob, (*ntts)[i], min_ntt, 2700);
}
...
}PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. tpplib kerneldensityrtmixturedistr.cxx 104
В процессе анализа белков возникают различные ошибки, о которых следует сообщить пользователю. Приведённый ниже код пытается сформировать сообщение, но, к сожалению, не может это сделать.
RAMPREAL *readPeaks(RAMPFILE *pFI,
ramp_fileoffset_t lScanIndex)
{
...
else
{
const char* pEndAttrValue;
pEndAttrValue = strchr( pBeginData +
strlen( "contentType=\"") + 1 , '\"' );
pEndAttrValue = '\0';
fprintf(stderr, "%s Unsupported content type\n" , pBeginData);
return NULL;
}
...
}PVS-Studio: V527 It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *pEndAttrValue = '\0'. tpplib ramp.cpp 1856
Этот код ищет в строке двойную кавычку и пытается записать на её место терминальный ноль. Ошибка заключается в том, что указатель pEndAttrValue не разыменовывается. Вместо записи нуля в нужное место, обнуляется указатель pEndAttrValue. Как следствие, пользователю будет распечатан лишний и ненужный текст.
Корректный вариант кода:
*pEndAttrValue = '\0';Аналогичные ошибки можно заметить ещё в некоторых местах:
При записи XML файлов требуется сформировать временный буфер из 10 байт. Первый байт буфера должен быть равен '1', а остальные байты обнулены. Для этих целей удобно использовать функцию strncpy(). Описание функции strncpy:
char *strncpy (char *dst, const char *src, size_t len);
dst — Destination string.
src — Source string.
len — Number of characters to be copied.
The strncpy function copies the initial count characters of strSource to strDest and returns strDest. If count is less than or equal to the length of strSource, a null character is not appended automatically to the copied string. If count is greater than the length of strSource, the destination string is padded with null characters up to length count.
В библиотеке XML написан корректный на первый взгляд код:
void Out2XML::writeOutData() {
...
// assume a string of less than
// 9 characters will represent the charge state
char *chg=(char*)malloc(10 * sizeof(char));
//zero-fill the rest of the array
strncpy(chg, "1", sizeof(chg));
...
}PVS-Studio: V579 The strncpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. CombineOut out2xml.cxx 210
Ошибка заключается в том, что оператор sizeof() возвращает размер указателя, а вовсе не размер буфера. Несколько байт в конце останутся неинициализированными. Корректный вариант кода:
strncpy(chg, "1", 10); // zero-fill the rest of the arrayАналогичная ошибка:
V579 The strncpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. CombineOut out2xml.cxx 214
Можно быстро проверить, что строка пустая, сравнив первый её символ с нулём. Пример: str[0] == '\0'. Так часто делаю, но, к сожалению, иногда забывают разыменовывать указатель. Эти ошибки выглядят так:
void SAXSpectraHandler::pushPeaks(....)
{
...
while(*pValue != '\0' && a < m_peaksCount) {
while(*pValue != '\0' && isspace(*pValue))
pValue++;
if(pValue == '\0')
break;
m_vfM.push_back((float)atof(pValue));
...
}PVS-Studio: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *pValue == '\0'. xtandem saxhandler.cpp 323
Второй оператор while() служит для того, чтобы пропустить все пробелы. Затем нужно узнать, есть ли что-то после пробелов. Но указатель 'pValue' не разыменовывается и проверка никогда не срабатывает.
Корректный код:
if(*pValue == '\0')Есть ещё парочка мест, где забыто разыменование указателя:
В библиотеке для работы с XML неаккуратно относятся к приватным данным. Не думаю, что это может как-то повредить TPP, однако раз ошибка найдена, её стоит описать.
Когда приватные данные (пароли, логины и т.п.) больше не нужны, их следует затереть в памяти. В противном случае они самым неожиданным образом могут попасть в файл, быть переданы по сети и так далее. Это не страшилки. Это реальность. Предлагаю познакомиться вот с этой статьёй, чтобы понять, как это может произойти: Перезаписывать память - зачем?
Чтобы уничтожить приватные данные в буфере, в него нужно что-то записать. Многие используют для этих целей функцию memset(). Это плохая идея. Компилятор вправе удалить её вызов, если результат её работы никак не используется. Подробнее данная тема рассмотрена в документации: V597.
Пример опасного кода:
void CSHA1::Final()
{
UINT_8 finalcount[8];
...
memset(finalcount, 0, 8);
Transform(m_state, m_buffer);
}PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. pwiz sha1.cpp 205
Компилятор вправе удалить вызов функции memset(), так как массив 'finalcount' после этого не используется.
Корректный код:
RtlSecureZeroMemory(finalcount, 8);
Аналогичные недоработки:
Класс DiscriminantFunction содержит виртуальные функции, но при этом деструктор не объявлен виртуальным.
V599 The virtual destructor is not present, although the 'DiscriminantFunction' class contains virtual functions. tpplib discrimvalmixturedistr.cxx 201
Анализатор выдал много предупреждений V595. Это означает, что указатель вначале используется, а затем проверяется на нулевое значение. Это подозрительно, но далеко не всегда является ошибкой. Очень часто, указатель никак не может быть в этом месте нулевым. А проверка на 0 просто лишняя.
К сожалению, я ленив и не стал выяснять, где может произойти разыменование нулевого указателя, а где нет. Прощу прощения :). Думаю те, кого заинтересует эта статья, смогут это сделает самостоятельно, скачав PVS-Studio. Ещё отмечу, что в статью вошли далеко не все фрагменты кода, которые могут содержать ошибку. Очень сложно анализировать совершенно незнакомый проект.
Когда я перечитал написанную статью, то у меня возникла ощущение, что после знакомства с ней у читателя может сложиться неправильное впечатление. Кажется, что статья выпячивает глупые ошибки и выставляет программистов не в лучшем свете. Нет, её смысл другой. Я хотел показать, что программирование это сложно. Очень сложно. Так сложно, что помимо алгоритмических ошибок можно наделать множество опечаток и прочих ляпов. И причина этих ляпов, вовсе не глупость или недостаточные знания. Слишком много при программировании приходится помнить и сосредотачиваться на множестве несвязанных задач. Всё это и приводит не только к сложным ошибкам, но и к простым. Со сложными может сразиться только программист. А вот многие простые, вполне может выявить инструмент статического анализа. При этом выявляя простые ошибки, он позволяет программисту большее внимание уделить алгоритмам и структуре программы. Не стоит игнорировать данный вид инструментария.
Получился длинный вывод. Поэтому ещё раз кратко и чётко:
0
0
0
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
0