понедельник, 5 сентября 2011 г.

[prog.bugs] Вернусь к незакрытому обсуждению бага в C++ном коде

Неожиданно обнаружил в почте комментарий к старой теме “А жуки все лезут и лезут. На этот раз приплюснутые”. К сожалению, тогда почему-то не смог ответить. А жаль, тема хорошая. Поэтому возвращусь к ней еще раз.

Итак, одна из проблем была вызвана кодом:

std::string
hex_char( char ch )
{
   char v[5];
   std::sprintf( v, "%02x", ch );
   return std::string(v);
}

Напомню суть проблемы. По умолчанию char в С++ – это знаковый тип (хотя это зависит от ключей компилятора). Поэтому, если в ch содержится не ASCII символ из диапазона [128, 255], то при передаче в sprintf будет получен отрицательный int. Шестнадцатиричное представление которого никак не уместится в буфер v (по крайней мере для int-ов, чья размерность больше 8 бит).

В тогдашних комментариях звучала мысль, что я зря обозвал этот баг приплюснутым. Это типичная C-шная ошибка – неверное указание размера буфера-приемника для sprintf (как и использование sprintf вместо snprintf, а еще лучше C++ных потоков вместо функций printf-ного семейства).

Посему объясняю свою точку зрения. Проблемы с sprintf – это уже следствие главной проблемы. А главная проблема в том, что разработчик забыл об особенностях C++ со знаковостью типа char. И, поскольку в C++ эта особенность присутствует изначально и человек пишет только на C++, то это именно проблема C++. То, что С++ унаследовал ее из C уже не имеет значения (хотя я C не знаю в достаточной степени, поэтому не уверен в наличии в нем заморочек на счет signed char, unsigned char).

Более того, я уверен, что человек, забывший об особенностях знака в типе char, отгреб бы гораздо больших проблем и с применением C++ных потоков. Вот, смотрим простейший код:

#include <iostream>

int
main()
   {
      char c = -1;

      std::cout << '\'' << std::hex
            << static_cast< unsigned int >(c) << '\'' << std::endl;
   }

Что мы увидим, запустив такую программу? ‘ff‘? А нетушки! Visual C++ и Intel C++ выдают `ffffffff`. Что совершено логично.

Но такой неправильный вывод, на самом-то деле, гораздо хуже аварийного завершения программы (по крайней мере во многих случаях). Представьте, что такое неправильное формирование hex-представление используется для сохранения в БД значения MD5 хеша или для передачи этого же хеша в XML-документе. Ведь ошибки не произойдет, hex-представление будет получено и уйдет куда-то дальше. Где со временем обязательно возникнут проблемы. Но ведь это же уже чужие проблемы ;)

Вот так я смотрю на это дело. И хорошо, что ошибка проявилась так жестко – падением программы. Ведь все-таки лучше сломаться, чем произвести неправильный результат. Fail fast, как говорится ;)

22 комментария:

Left комментирует...

все эти рассуждения имеют право на жизнь если бы ошибка приводила к бросанию unhabdled exception. Здесь же ситуация в стотыщмильйонов раз хуже - это перепонение буфера которое даёт возможность для атаки злоумышленника на компьютер (хотя и достаточно сложноосуществимую). И именно даже за самую потенциальную возможность осуществления такой атаки разработчика надо жестоко бить линейкой по пальцам.

Left комментирует...

да, да, я знаю про guard-ы которые вставляет компилятор MSVC, и то что в данном конкретном случае уязвимость использовать не получится. Но кто даст гарантии что завтра код не портируют под какую-то экзотическую платформу гда именно этот кусок станет троянской лошадкой для того чтобы наделать бед на миллионы долларов? Страх перед переполнением буфера должен быть у нативного программиста в крови.

Андрей Валяев комментирует...

По этому поводу вспомнилось... :)

http://mdf-i.blogspot.com/2010/01/blog-post_21.html

Евгений Охотников комментирует...

@Left:

>И именно даже за самую потенциальную возможность осуществления такой атаки разработчика надо жестоко бить линейкой по пальцам.

Именно поэтому я и сделал оговорку "по крайней мере во многих случаях". Смотрящий наружу код должен требовать более пристального к себе внимания.

На счет того, что C++ (как и C) слишком уязвим, я не спорю. Но в данном случае изначальная причина всех бед вовсе не в sprintf.

Евгений Охотников комментирует...

@Андрей Валяев:

Отличный пример! Супер!
Именно об этом я и говорил. И ведь, что не маловажно, в Сбере же софт тестирование и приемку прошел. А если бы софт тупо ломался -- не прошел бы и ты бы такого чека не получил.

Eugeniy комментирует...

Получается так:
1. sprintf - расстрел памяти, UB. Ошибка может быть обнаружена сразу, а может очень и очень нескоро, причем в совершенно другом месте. Ошибка может быть в релизе и может отсутствовать в дебажной версии. В общем, кошмар :)

2. потоки - получаем не то, что ожидали. Ну так с тем же успехом можно манипулятор hex пропустить... В ряде случаев такая ошибка также может быть обнаружена весьма нескоро. Однако локализовать ее проще в разы.

Вывод из всего этого - баг действительно С++; однако если писать в стиле С++, то поправить его много проще.

Евгений Охотников комментирует...

@Eugeniy:

К сожалению, я не уверен в том, что если писать в стиле C++, то поправить его будет проще.

Андрей Валяев комментирует...

Здесь наверное только тестирование способно помочь. В первую очередь автоматизированное.

На уровне кода этого по моему никак не отследишь.

Евгений Охотников комментирует...

@Андрей Валяев:

>Здесь наверное только тестирование способно помочь.

В принципе да. У меня одна из первых мыслей была -- "как же эта утилита тестировалась, что баг ни разу не проявился?"

Хотя, практика показывает, что для тчательного покрытия тестами думать нужно не меньше, а то и больше, чем при написании кода. Наверное, в таком деле могут помочь автоматические генераторы тестов, которые сами прогоняют код через различные граничные условия. Но мы такими (еще?) не пользуемся.

Left комментирует...

> Смотрящий наружу код должен требовать более пристального к себе внимания.

Этот код может не торчать наружу, и тогда отследить такую уязвимость при code review будет только сложнее. Поэтому на каждое использование sprintf вместо snprintf разработчик должен получить письменное согласие не менее чем пяти человек, один из которых обязательно должен быть Херб Саттер.

Евгений Охотников комментирует...

@Left:

AFAIK, snprintf является частью стандарта C99, а не C89. Следовательно, она не является частью стандартов С++ 98 и 2003. Посему можно запросто отгрести проблем при портировании C++ кода с snprintf. В отличии от sprintf.

К сожалению...

Left комментирует...

Нет snprintf - напиши свою или найди гуглом готовую. Суть не в конкретной функции snprintf а вообще в использовании функций которые не проверяют границы переданного буфера. На каждый переданный буфер без переданного размера дожен звенеть звоночек в голове.

Евгений Охотников комментирует...

@Left:

Я уверен, что для кода:

unsigned char ch;...
char buf[3];
std::sprintf(buf, "%02x", ch);

никаких проверок не нужно.

А вот что проще выдресировать -- звенеть звоночку при использовании sprintf или же писать идеоматичный код, который в проверках не нуждается... ХЗ.

Eugeniy комментирует...

>> К сожалению, я не уверен в том, что если писать в стиле C++, то поправить его будет проще.

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

А если писать в стиле С++, будет "обычная" логическая ошибка. Ловится тестированием - ручным или автоматическим. И, ИМХО, в среднем намного проще локализуется

К тому ж, средства С++ при последовательном применении позволяют полностью убрать определенный класс ошибок. Так почему бы из и не использовать?

Евгений Охотников комментирует...

>К тому ж, средства С++ при последовательном применении позволяют полностью убрать определенный класс ошибок. Так почему бы из и не использовать?

Я сам такого же мнения. Поэтому и перешел давным давно с C на C++, о чем и не жалею.

Просто в данном конкретном случае для меня не факт, что ошибка в коде, написанном в C++ном стиле, было бы проще обнаружить и локализовать.

имя комментирует...

насчтет с++ по-моему ты погорячился

вот что будет результатом такого:

unsigned char x=255;

unsigned char y=(x<<1)>>1;

Евгений Охотников комментирует...

@имя:

А ты уверен, что это проблема именно C++? По-моему, это в большей степени зависит не от языка, а от степени умности компилятора. Если компилятор слишком умен, чтобы понять, что выражение (x/2)*2 эквивалентно x*2/2, что эквивалентно x*1, что эквивалентно x, то без особой разницы, на каком языке записано (x/2)*2. Это я к тому, что Intel C++ вообще генерирует код в котором есть только вызов printf c параметром 255 (я про маленькую тестову програмку, которая делает сдвиги и печатает результат printf-ом).

Хотя, подозреваю, ты имел в виду, что результат (x<<1) это уже не unsigned char, а unsigned int (или unsigned short), поэтому последующий сдвиг вправо возвращает единичку, которая должна была быть потеряна, на место. Поэтому правильный результат гарантировано получается через: y = static_cast<unsigned char>(x<<1)>>1;

Но, кстати, и в этом случае Intel C++ оптимизирует все так, что вычисления выполняются в compile time и в код подставляется уже результирующая константа.

имя комментирует...

совершенно верно -- я хотел поговорить про потерянную единичку, но написал слегка неверно (позволив это статически вычислить), что сделало вопрос еще интереснее

а вот смотри:

#include

main()
{
unsigned int x=4000111222u;
unsigned int y=(x<<1)>>1;
unsigned int z=(x*2)>>1;
std::cout << y << ' ' << z << '\n';
}

у меня жцц даже при -О3 выдает

1852627574 1852627574

подозреваю, что интел выдаст ТО ЖЕ и будет прав

имя комментирует...

к чему я это затеял -- к тому, чтобы сказать, что char и short -- это НЕ типы данных, а мы их воспринимаем как типы данных и из-за этого происходят ошибки навроде твоей (ведь ты интуитивно воспринмал свой код как "распечатать char с форматом х", так ведь?)

char и short не типы, так как СВОИХ операций у них нет -- их можно только закинуть в регистр и выкинуть (с обрезкой) из регистра

И ВСЕ!!! (хотя я все же не эксперт по с/с++ и могу ошибаться)

char и short это что-то вроде спецификатора объема хранения int

Евгений Охотников комментирует...

@имя:

Intel C++ так же выдал 1852627574. Причем со включенной оптимизацией он так же не сгенерировал кода по вычислению этого значения в run-time. Произвел вычисления в compile-time и вставил в код результирующую константу.

Но, я полагаю, что это проблема не столько C++. Сколько вообще вычислений на компьютере, где, в отличии от математики, числа имеют конечные размерности. Кстати из-за этого даже пришлось вводить отдельную дисциплину -- вычислительную математику -- т.к. выяснилось, что чистые математики не отдают себе отчета о проблемах вычислений с помощью компьютеров ;)

На счет типов char, short и т.д. я соглашусь если речь идет только о вычислениях. Например, при работе с указателями char, short и int вполне себе разные типы. Опять же, когда приходится перепаковывать байты в каких-нибудь бинарных протоколах, то unsigned char это вполне себе конкретный тип, очень сильно отличающийся от int-а.

Евгений Охотников комментирует...

@имя:

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

Например, в C++ ты можешь написать шаблон для функции перепаковки представления числа из машинного представления в BigEndian. И сделать ее специализации для short, int. В которых будет встроенный контроль за размерностью типа (строго 2 и 4 байта).

И ты гарантированно будешь получать по рукам от компилятора за попытки использования этих функций с другими типами -- вроде float, double или long long. Что намного лучше, чем иметь проблемы с несовпадением параметров в форматной строке printf с типом реального аргумента.

имя комментирует...

я собственно хотел этим доказать, что компилятор, когда не потерял единичку, руководствовался вовсе не тем, что "выражение (x/2)*2 эквивалентно x*2/2, что эквивалентно x*1, что эквивалентно x" -- т.к. в случае с интами он эту единичку успешно потерял

так что пойнт в том, что и у компилятора тоже нет char-ов, нет short-ов, и нет математических натуральных чисел -- а есть инт

и это -- наличие только инта -- именно сишное наследство (а вовсе не плюсовое); в языке на смену с++ его надо удалить

(кстати -- если бы вдруг компилятор точно считал то, что в рантайме считается приближенно, то это было бы плохо -- и поэтому я надеялся, что интел даст тот же результат, что и жцц; впрочем, иногда все же разница бывает, но это вроде как в стандарте описано и жцц дает варнинг)