25 мая 2010 г.

Если что-то работает - это ещё не значит, что это работает правильно

a.k.a. "Если что-то работает, это ещё не значит, что это вообще должно работать".

Примеры из жизни.

Пример 0.

Как вам такой код?
procedure TForm1.Button6Click(Sender: TObject);
var
 s:string;
begin 
 s:=DateToStr(Now); \\ чтоб к имени прибавлялась дата
 EF.ActiveWorkbook.SaveAs(Form1.Edit.Text + s[1] + s[2] + '_' + s[4] + s[5] + '_' + s[7] + s[8] + s[9] + s[10]); 
end;
Рабочий код? Рабочий. Ведь тут "код подошёл, всё работает, спасибо!".

Хороший ли это код? Да ни в коем разе!

Тут сразу несколько проблем. Первое что бросается в глаза - посимвольная склейка. Почему не склеивать сразу строки? Но это так, придирка. Настоящая проблема - неявное предположение о фиксированном формате даты. DateToStr переводит дату в строку, основываясь на текущих региональных установках. Которые могут меняться.

Что означает, что год у меня на машине может быть представлен двумя цифрами, а не четырьмя - и тогда этот код красиво вылетит с Access Violation или выведет мусор. А ещё год может идти не последним числом, а первым.

Как это исправить? Ну, вы можете зафиксировать формат даты-времени, вызвав перегруженный вариант DateToStr, принимающий дополнительный параметр - настройки форматирования. Но гораздо лучшим решением будет использование функции вроде FormatDateTime:
procedure TForm1.Button6Click(Sender: TObject);
var
  s: string;
begin 
  s := FormatDateTime('dd_mm_yyyy', Now); // чтоб к имени прибавлялась дата
  EF.ActiveWorkbook.SaveAs(Form1.Edit.Text + s); 
end;
Очень просто. Код ещё не идеален, но хотя бы он будет работать корректно в любых условиях (и, кроме того, теперь он выглядит красивее и понятнее!).

Пример 1.

Начнём с этого - всем известная реализация Base64 на http://www.delphi3000.com/ от Daniel Wischnewski из Delphi-PRAXiS - далеко не "noname" товарищ. Исходник имеет рейтинг 9/10. Разошёлся по многим FAQ и используется в куче программ (в том числе, он использовался в EurekaLog).

Казалось бы, что может пойти не так?

Оказывается, что эта реализация вообще не работает (под этим подразумевается, что она не работает корректно, иными словами, не должна работать вообще). Конкретно: этот код содержит memory corruption bug. Ещё конкретнее: в Base64Encode, третья строка "mov EAX, EBX" - какой-такой ещё EBX? Он неопределён. Параметры в register-процедуру передаются в регистрах EAX, ECX и EDX. EBX же вообще не связывается с параметрами, при входе в функцию значение регистра EBX - произвольно. Правильный вариант кода выглядит, например, так: "mov EAX, InSize" (как это и сделано в Base64Decode).

Столкнулся я с этим в поддержке EurekaLog. Мы получили баг-отчёт с memory corruption на ровном месте. К счастью была предоставлена воспроизводимая демка, и я по ней вышел на этот код, который когда-то был взят с Delphi3000 и встроен в EurekaLog. Вместе с багом. Удивительно, что это работало несколько лет. Ещё удивительнее, что никто, использующий этот код или его копии из FAQ, за 7 лет не нашёл этой ошибки. Окей, наверняка кто-то сталкивался с загадочными багами, но причину найти не могли. Вот она, сила Copy&Paste.

Конкретно в варианте с EurekaLog EBX наследовался из вызывающего, где он оказывался равен OutSize из Base64EncodeStr (вместо InSize; InSize <= OutSize). Соответственно, Base64Encode кодировала больше данных, чем было нужно (заодно портя память, т.к. выходной буфер был предназначен только для содержания кодированных InSize байт, а не OutSize байт). Например, вместо взятия, скажем, 11-ти байт с входного буфера и записи их в 16-ти байтный выходной буфер, эта функция брала 16 байт из исходного буфера (ага, шанс на AV не сработал) и записывала 24 байта в выходной буфер (который имел размер 16 байт). Да, алгоритм работал, да он выполнял кодирование/декодирование. Но работал неверно - он портил память. Но это работало. В 99.9% случаев всё "просто работало" и не было никаких ошибок или вылетов. Но иногда ваше приложение вылетало. Причём в месте, совершенно никак не связанным с исходной проблемой. Это был какой-то совершенно другой код.

Ситуацию в этом случае усложняет то, что пример написан на ассемблере, что затрудняет его чтение и анализ.

Вот корректный вариант реализации Base64, который использует только Паскаль и, более того, работает быстрее вышеуказанной ассемблерной реализации. Не забывайте только, что этот код, попадая под определение "опубликовано в интернете" - говно.

Примечание: кажется, не все понимают, зачем я выложил этот код. Код выложен только как пример исправления. Это не означает, что его нужно брать и использовать. Он не предназначен для этого. Вместо него нужно использовать стандартный модуль EncdDecd (в новых версиях Delphi - Soap.EncdDecd).

Пример 2 - посыл сообщений фиксированному недокументированному окну будет работать... до следующего обновления системы! Ну, конечно же вы будете винить "кривую Windows", а не свою программу.

Пример 3 - вы ошибочно передаёте GetDesktopWindow куда-то вместо 0. Это будет работать... пока вызывающий не захочет показать окно. Это может не происходить у вас, но происходить на машине вашего друга. Или не происходить в XP, но происходить в Windows 2000. И снова: будете ли вы винить свою программу или "кривую Windows, написанную глупым Биллом Гейтсом на коленке"?

Пример 4 (в четырёх частях). В DllMain чрезвычайно сложно сделать всё правильно. Если вы что-то писали в initialization или begin/end - почти наверняка вы уже облажались. Если ваши DLL при этом работают - то только благодаря случайности, как и в предыдущих примерах. Но иногда они будут вылетать. А вот и пример. Довольно искусственный, зато хорошо воспроизводимый.

Пример 5 и его очень похожий вариант - вы можете ошибиться в сигнатуре функции и это будет работать, пока... не изменится какая-то маленькая деталь в вызывающем. Иногда достаточно просто пересобрать проект с другими настройками или другой версии компилятора, чтобы работающая программа сломалась бы. Посмотрите по ссылке пример с RunDLL32 - как часто вы сами видели такое во всяких "полезных советах" и "скрытых возможностях Windows"?

Пример 6 - если вы не позаботитесь о выравнивании ваших данных, то ваш код будет работать. Но очень редко выдавать неверные результаты. Удачи вам поймать эту ситуацию под отладчиком!

Это не говоря уже про такие тривиальные вещи. Вот ещё пример. И ещё. Если вы использовали в своём коде недокументированную возможность, то как бы удобна и нужна она ни была - откажитесь от неё. Потому что иначе ваша программа перестанет работать с выходом новой версии ОС или даже просто сервис пака.

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

Мораль истории: когда кто-то говорит "у меня это работает, спасибо", опасайтесь: тут что-то не так.

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

Читать далее: 90% кода в интернете - говно.

11 комментариев :

  1. Огромное спасибо за первый пример.

    ОтветитьУдалить
  2. Регистр EBX не является свободным регистром.
    Его значение на выходе из sub-процедуры должно быть таким же как и на входе в эту sub-процедуру. В противном случае вызывающий ее код может накрыться медным тазом.

    In general, the rules of register use in an asm statement are the same as those of an external procedure or function. An asm statement must preserve the EDI, ESI, ESP, EBP, and EBX registers..

    ОтветитьУдалить
  3. Да, неточно выразился. Имел ввиду, что EBX не связан с аргументами.

    ОтветитьУдалить
  4. Всё так. Но! Кому на самом деле нужен вменяемый код? "Пятилетка за три дня" - совсем другое дело. Никто не будет ждать тоже рабочий, но вменяемый код (тем более окажется что завтра оно никому не нужно, нужно было вчера). Когда кажется что всё работает и написано корректно - всё равно за несколько лет любой, даже классный, программист сделает несколько фиксов.

    Да и потом кривой код даст работу целой армии программистов. Которые будут писать то же, но рабочее. Или годами фиксить, выпуская новые версии. Подсадят на оплату потому что "регистрированным патчи".

    Вот еще тема "Можно ли скрыть кривость кода за морем вменяемых комментариев в коде?". Возможно что в случае Base64 за счет комментариев "3 к одному" код и не был виден.

    ОтветитьУдалить
  5. уважаемый Александр !

    Большое вам спасибо за надёжный код модуля Base64.
    Я облазил весь Интернет в поисках хорошей реализации кодирования/декодирования base64, но нормальный рабочий код нашёл только у вас.

    Не могли бы вы также выложить проверенный и надёжный модуль для асимметричного шифрования RSA ?

    ОтветитьУдалить
  6. Смысл поста в "думайте своей головой, а не копируйте код". И код с Base64 выложен только для примера, сравнения с исходной реализацией. Он не предназначен для бездумного включения в свои проекты.

    Для включения в проекты предназначен код, находящийся в библиотеках кода - например, JCL, JVCL, JWSCL, Abbrevia, DCPCrypt и т.п.

    ОтветитьУдалить
  7. Хотел добавить пару слов насчёт юнита Base64 ("Он не предназначен для бездумного включения в свои проекты")

    Я включил его в свой проект вовсе не бездумно. Я очень долго думал: "включать его в проект или не включать?", а потом принял решение: "включать!". И работает он нормально (ещё раз вам спасибо за этот модуль).

    Да, я примерно представляю, что вы на это ответите.
    Да, я читал вашу интереснейшую статью "90 % кода в интернете - фигня" и очень даже согласен с ней.

    Но понимаете, какая штука выходит - не может каждый программист заново для себя писать с нуля операционную систему, ввод-вывод, файловую систему, сетевые протоколы и т.д. В том числе и Base64 и RSA.

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

    Думаю, что стоит слегка сместить акценты с "вдумчиво---или---бездумно", на "рабочий хороший код---или---фигня как base64 у Daniel Wischnewski".

    Думаю, также, что многие специалисты склонны больше доверять вам, как признанному сообществом программистов профессионалу, нежели искать в коде завуалированные ошибки, судорожно водя по экрану дрожащим пальцем, бормоча вполголоса строки кода RSA, AES, Base64 (или чего-там) и пытаясь вспомнить (хотя бы приблизительно) основные теоретические положения из учебника алгебры за 6-й класс (а заодно и таблицу умножения).

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

    Вот я и хотел вас попросить выложить также хороший и надёжный код реализации RSA.

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

    А самому вникать во все тонкости RSA, AES, Base64 и т.д. - таких Дональдов Кнутов, наверное, от силы человек десять наберётся на всей планете.

    ОтветитьУдалить
  8. Ну, я отвечу вот этой ссылкой: http://www.gunsmoker.ru/2010/05/90.html

    Использовать надо не кусочки кода, а уже написанные и вылизанные библиотеки. Джедаи, DCPCrypt и т.д. И не копировать, а подключать.

    ОтветитьУдалить
  9. Кстати по поводу правильной реализации base64.
    Зачем использовать сторонние реализации, когда все уже давно реализовано в модуле EncdDecd?
    Достаточно его подключить и пользоваться :)

    ОтветитьУдалить
  10. Ну, этого модуля нет в совсем уж старых версиях Delphi. А для более новых - ничто не мешает. Но про него надо знать, конечно же.

    ОтветитьУдалить
  11. Не люблю я «разглючки», хотя приходится иногда.

    С датой вы меня рассмешили… Буквально на днях одна «прекрасная леди» шлёт мне запрос на SQL: что в нём не так? Она «руками» переформатирует дату из YYYY-MM-DD в MM/DD/YY. Очередная версия СУБД отказалась от неявного перевода дат в строки (а может, перевела, но в другой формат — хрѣнъ уже его поймёт) — вот вам и ошибка.

    Со знанием библиотеки — тяжело это, скажу прямо, когда надо копать от забора и до обеда. Мой личный косяк: http://ithappens.ru/story/3913

    ОтветитьУдалить

Можно использовать некоторые HTML-теги, например:

<b>Жирный</b>
<i>Курсив</i>
<a href="http://www.example.com/">Ссылка</a>

Вам необязательно регистрироваться для комментирования - для этого просто выберите из списка "Анонимный" (для анонимного комментария) или "Имя/URL" (для указания вашего имени и (опционально) ссылки на сайт). Все прочие варианты потребуют от вас входа в вашу учётку.

Пожалуйста, по возможности используйте "Имя/URL" вместо "Анонимный". URL можно просто не указывать.

Ваше сообщение может быть помечено как спам спам-фильтром - не волнуйтесь, оно появится после проверки администратором.

Примечание. Отправлять комментарии могут только участники этого блога.