LibRaw, Coverity SCAN, PVS-Studio

    LibRaw and PVS-Studio
    Прочитал заметку о проверке маленького проекта LibRaw с помощью Coverity SCAN. Из статьи следует, что ничего интересного не нашлось. Решил попробовать, сможет ли найти что-то анализатор PVS-Studio.

    LibRaw


    LibRaw это библиотека для чтения RAW-файлов, получаемых с цифровых фотокамер (CRW/CR2, NEF, RAF, DNG и других). Сайт: http://www.libraw.org/

    Проверка с помощью Coverity SCAN


    А вот статья, которая подвигла меня на проверку проекта с помощью PVS-Studio: "Про статический анализ С++". Кратко процитирую основную часть статьи:

    Coverity SCAN: 107 предупреждений, из них где-то треть — с High Impact.

    Из High Impact:

    Штук 10 в Microsoft STL

    Еще какое-то количество такого примерно вида:
    int variable;
    if(layout==Layout1) variable=value1;
    if(layout==Layout2) variable=value2;

    И на это дается предупреждение, дескать неаккуратненько, не инициализированная переменная. Я с ним согласен по общим ощущениям, так не надо делать. Но в реальной жизни бывает два вида layout — и это явно прописано в вызывающем коде. Т.е. у машинки достаточно данных, чтобы сообразить, что это не 'High impact', а просто неаккуратненько.

    Некоторое количество предупреждений, дескать unsigned short при расширении до 32-64 бит может больно укусить. С этим я не хочу спорить — формально машинка права, а по факту в этих unsigned short живут размеры картинки и до 32767 они в ближайшие годы не дорастут.

    Т.е. опять, фиксить не надо — в случае данной предметной области.

    Все остальные найденные 'High Impact' проблемы — это просто ложные срабатывания. Т.е. код, согласен, не идеальный (видели бы вы этот код из dcraw !), но все найденное — не является ошибкой.

    Проверка с помощью PVS-Studio


    Теперь посмотрим, удастся ли что-то найти после Coverity анализатору PVS-Studio. Конечно, никаких ожиданий об обнаружении супер-ошибок нет, но всё равно интересно попробовать.

    Анализатор PVS-Studio выдал 46 предупреждений общего назначения (первый и второй уровень важности).

    Предлагаю посмотреть на фрагменты кода, которые показалось мне интересными.

    Опечатки


    void DHT::hide_hots() {
      ....
      for (int k = -2; k < 3; k += 2)
        for (int m = -2; m < 3; m += 2)
          if (m == 0 && m == 0)
            continue;
          else
            avg += nraw[nr_offset(y + k, x + m)][kc];
    
      ....
    }

    Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: m == 0 && m == 0 dht_demosaic.cpp 260

    Видимо имеем дело с опечаткой. Скорее всего, проверка должна была быть такой:
    if (k == 0 && m == 0)

    Идентичный фрагмент также имеется в файле aahd_demosaic.cpp (строка 199).

    Приоритет операций


    int main(int argc, char *argv[])
    {
      int ret;
      ....
      if( (ret = RawProcessor.open_buffer(iobuffer,st.st_size)
                 != LIBRAW_SUCCESS))
      {
        fprintf(stderr,"Cannot open_buffer %s: %s\n",
          argv[arg],libraw_strerror(ret));
        free(iobuffer);
        continue;
      }
      ....
    }

    Предупреждение PVS-Studio: V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. dcraw_emu.cpp 468

    Ошибка, связанная с приоритетами операций. В начале выполняется сравнение «RawProcessor.open_buffer(iobuffer,st.st_size) != LIBRAW_SUCCESS». Затем результат этого сравнения записывается в переменную 'ret'. Если возникнет ошибка, то в файл будет распечатан неправильный код ошибки. Не критичный недочёт, но всё равно стоит про него рассказать.

    Сдвиг отрицательных чисел


    unsigned CLASS pana_bits (int nbits)
    {
      .... 
      return (buf[byte] | buf[byte+1] << 8) >>
             (vbits & 7) & ~(-1 << nbits);
      ....
    }

    Предупреждение PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. dcraw_common.cpp 1827

    Сдвиг отрицательных значений приводит к undefined behavior. Такими трюками часто пользуются, и программа делает вид что работает. Но на самом деле, полагаться на такой код нельзя. Подробнее про сдвиги отрицательных чисел можно прочитать здесь: Не зная брода, не лезь в воду. Часть третья.

    Аналогичные сдвиги можно найти здесь:
    • dcraw_common.cpp 1851
    • dcraw_common.cpp 2085
    • dcraw_common.cpp 2814
    • dcraw_common.cpp 6644

    Странные фрагменты


    void DHT::illustrate_dline(int i) {
      ....
        int l = ndir[nr_offset(y, x)] & 8;
        l >>= 3;
        l = 1;
      ....
    }

    Предупреждение PVS-Studio: V519 The 'l' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 671, 672. dht_demosaic.cpp 672

    Возможно это не ошибка и «l = 1» написано специально. Однако, код выглядит подозрительно.

    Вот ещё одно подозрительное место:
    void CLASS identify()
    {
      ....
      if (!load_raw && (maximum = 0xfff))
      ....
    }

    Предупреждение PVS-Studio: V560 A part of conditional expression is always true: ((imgdata.color.maximum) = 0xfff). dcraw_common.cpp 8496

    Заключение


    Оба анализатора нашли очень мало. Это естественно для маленьких проектов. Однако, всё равно было интересно провести эксперимент по проверке LibRaw.

    PVS-Studio

    340,62

    Ищем ошибки в C, C++ и C# на Windows, Linux, macOS

    Поделиться публикацией
    Комментарии 36
      +18
      Ага, спасибо.

      DHT/AAHD — это новый (contributed) код, я передам автору.
      Последнее — не ошибка (стиль, понятно, херовый, но там унаследованный код, который «весь такой»)

      Про сдвиги отрицательных чисел — подумаю. Это опять унаследованный код, но так как LibRaw начали нести с Intel на всякие другие архитектуры — может быть стоит вместо -1 явно написать 0xffffff
        –1
        Хороший вариант: (~0u)
          +3
          В некоторых случаях лучше написать явно. Компилятору пофиг, а читающему — приятнее.
            –2
            Не знаю какой точно тип у maximum, но как же переносимость?
              +3
              При чём здесь maximum…
              Обсуждалось выражение (-1 << nbits):
              Про сдвиги отрицательных чисел — подумаю. Это опять унаследованный код, но так как LibRaw начали нести с Intel на всякие другие архитектуры — может быть стоит вместо -1 явно написать 0xffffff

              Я предложил заменить -1 на (~0u), чтобы избавиться от неопределённого поведения. Напомню:

              2. The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled. If E1 has an unsigned type, the value of the result is E1 * 2^E2, reduced modulo one more than the maximum value representable in the result type. Otherwise, if E1 has a signed type and non-negative value, and E1*2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.

              Кто-то, не разобравшись, начал минусовать комментарий. Люди ведь могут подумать, что я неправду написал. Прошу читать мои статьи и комментарии внимательно.
                +3
                Я думаю, кто-то просто спросонья не понял, что это означает. А вообще инверсия беззнакового нуля — это действительно гарантированый способ получить единичку во все поляразряды, сколько бы их ни было.

                Пользуясь случаем, добавлю, что этот пост — первый раз когда я увидел реальную пользу от вашего продукта. Приятно видеть, что вы помогаете людям (в том числе проектам, которыми я пользуюсь) :)
                  0
                  Прошу прощения, спросонья не понял, подумалось, что про другой кусок кода речь.

                  Вы правы в обоих случаях — сдвиг отрицательных чисел это UB, ну и (~0u) однозначно лучше.

                  А в комментарии моём имелось ввиду, что: «0x… не известно сколько F» это и читается плохо, и с переносимостью проблемы будут гарантированы.
              +2
              А вот просто интересно услышать аргументы тех, кто ставит минусы для (~0u). Что не понравилось?
                +1
                Что не понравилось?

                Умный слишком? :-)
              +2
              > DHT/AAHD — это новый (contributed) код, я передам автору.

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

              После запуска статического анализатора, чаще всего ляпы будут найдены в новом коде или в крайне редко используемом коде (например, обработчиках ошибок). А остальное уже поправлено. Это я делаю намек на то, что настоящая польза от анализа будет получена при регулярных проверках, а не при «разовых наскоках».
              +8
              Волшебная хабрасила — автор проекта ответил на замечания через 15 минут :)
                +6
                А мне автор поста в моем блоге в комментариях написал, что вот есть пост на хабре.
                  +28
                  Слава богу, здесь не будет вопроса, сообщил ли я авторам библиотеки о найденных недочётах. :)
                    +3
                    Но есть еще много незаданных вопросов…
                      +39
                      Не оставим, им шанса.
                      Евгений, почему пока нет версии под Linux?
                        +41
                        И кстати почему так дорого?
                          +2
                          И есть ли скидка для опенсорсных проектов!?
                            0
                            Есть, пишите разработчикам ;)
                  0
                  мир тесен
                  +2
                  cppcheck говорит что [LibRaw-0.16.0/internal/dcraw_common.cpp:265]: (error) Uninitialized variable: c

                      244 void CLASS canon_600_coeff()
                      245 {
                      246   static const short table[6][12] = {
                      247     { -190,702,-1878,2390,   1861,-1349,905,-393, -432,944,2617,-2105  },
                      248     { -1203,1715,-1136,1648, 1388,-876,267,245,  -1641,2153,3921,-3409 },
                      249     { -615,1127,-1563,2075,  1437,-925,509,3,     -756,1268,2519,-2007 },
                      250     { -190,702,-1886,2398,   2153,-1641,763,-251, -452,964,3040,-2528  },
                      251     { -190,702,-1878,2390,   1861,-1349,905,-393, -432,944,2617,-2105  },
                      252     { -807,1319,-1785,2297,  1388,-876,769,-257,  -230,742,2067,-1555  } };
                      253   int t=0, i, c;
                      254   float mc, yc;
                      255 
                      256   mc = pre_mul[1] / pre_mul[2];
                      257   yc = pre_mul[3] / pre_mul[2];
                      258   if (mc > 1 && mc <= 1.28 && yc < 0.8789) t=1;
                      259   if (mc > 1.28 && mc <= 2) {
                      260     if  (yc < 0.8789) t=3;
                      261     else if (yc <= 2) t=4;
                      262   }
                      263   if (flash_used) t=5;
                      264   for (raw_color = i=0; i < 3; i++)
                      265     FORCC rgb_cam[i][c] = table[t][i*4 + c] / 1024.0;
                      266 }
                  
                  


                  и судя повсему действительно не инициализирована… почему cppcat ничего не сказал об этом?
                    +14
                    почему cppcat ничего не сказал об этом?
                    Потому, что CppCat лучше, чем Cppcheck.

                    Он раскрывает макрос FORCC и видит, что это:

                    #define FORC(cnt) for (c=0; c < cnt; c++)
                    #define FORCC FORC(colors)
                    

                    При раскрытии кода получаем:

                    for (raw_color = i=0; i < 3; i++)
                      for (c=0; c < colors; c++)
                        rgb_cam[i][c] = table[t][i*4 + c] / 1024.0;
                    


                    Спасибо, что помогли порекламироваться. :)
                      +1
                      мне же не жалко, для хорошего человека то :)
                        0
                        >#define FORC(cnt) for (c=0; c < cnt; c++)

                        это же ужасно, за что разработчик так ненавидит тех кто позже будет сопровождать код… главное непонятно где тут выигрыш…
                          0
                          А пристрастие к такому стилю программирования обязательно рано или поздно приведёт к появлению в коде после препроцессинга какого-нибудь бреда вроде:
                          if(strcmp(strdup(a), strdup(b)) == 0)
                          

                          Как правило сторонние разработчики, когда делают патчи, не всегда вдаются в тонкости устройства чужих дефайнов.

                          И такие ошибки cppcheck уже находил.
                            +2
                            LibRaw — исторически — это перефигаченный полуавтоматическими средствами dcraw.c, чтобы из утилиты сделалась библиотека со стабильным интерфейсом. Потому что иначе «весь мир», ругаясь и матерясь, импортирует в себя эту самую dcraw.c (см. например UFRaw, RawTherapee из опенсорного. Точно ACDsee из коммерческого).

                            Как следствие, там где модификация исходного кода dcraw не была строго необходимой — она не делалась. Потому что безпроблемность/автоматичность импорта — важнее всех прочих соображений.

                            В сопровождение же кода кем-то, кто заглянул на полчасика со стороны — в данном случае я верю слабо.

                          0
                          Cppcheck из git HEAD уже не рапортует о такой ошибке:)

                          А вообще там есть ещё много подозрительных мест, так что провериться cppcheck-ом не мешает.

                          Ошибки вида (ret = f()) уже давно компиляторы отстреливают, это говорит лишь о том, что никто не читает предупреждения компилятора:)
                            +1
                            Ошибки вида (ret = f()) уже давно компиляторы отстреливают, это говорит лишь о том, что никто не читает предупреждения компилятора:)
                            Может быть, я что-то делаю не так, но если просто открыть проект LibRaw, можно увидеть, что в настройках стоит /W0 (отключить все предупреждения).
                          0
                          и кстати да, почему так дорого?
                            0
                            Я дико извиняюсь, а что не дорого?
                              0
                              У них ж оупенсоурс, они таким проектам вообще бесплатно вроде давали.
                                0
                                эт я не знаю, я просил когда то натравить на наш проект PVS-Studio, но что то не взлетело…
                                  0
                                  Да, мы общались про Midnight Commander в начале 2011 года. В то время проверка Linux проектов у нас была не очень (никак). С тех пор прошло много времени. Попробуйте Standalone версию PVS-Studio.
                                +8
                                Можете проверить nginx? Было бы интересно почитать.
                                  +7
                                  Кругом много проектов. Не обещаю, но может быть.
                                  0
                                  Чтобы было и тут тоже.
                                  То что «признано» — внесено в бранч 0.16-stable у LibRaw и будет смержено в мастер (точнее, в приватной копии — уже, в паблик запушим как придет время).
                                  То что «не ошибки, просто стиль такой» — оставлено как есть, см. комментарии выше.

                                  Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                  Самое читаемое