Необходимое зло / Habr

Небольшое введение

Все мы любим делать вещи правильно. В интернете можно найти много статей с названием вроде "10 антипаттернов <...>", и, когда я пришёл на свою первую работу разработчиком, я думал, что из этих статей понял, как делать правильно, а как нет. К сожалению, реальность не всегда делится на плохое и хорошее, и некоторые вещи, которые встречаются в подобных статьях, всё-таки могут принести большую пользу при разработке.

Зачем эта статья?

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

Опираться я буду на прекрасную статью компании PVS-Studio как на самую полную и близкую к моему стеку.

Оператор goto

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

Когда может быть полезно

Оператор goto в некоторых случаях, по моему мнению, может улучшить читаемость кода. Рассмотрим фрагмент кода для выполнения GET-запроса с помощью библиотеки winhttp

#include <winhttp.h>

bool SendRequest(<...>){
  HINTERNET internet = WinHttpOpen(<...>);
  if (internet){
    HINTERNET connect = WinHttpConnect(<...>);
    if(connect){
      HINTERNET request = WinHttpOpenRequest<...>);	
      if(request){
        <...> <---Какие-то промежуточные действия
          WinHttpCloseHandle(request);
      }
      <...>
      WinHttpCloseHandle(connect);
    }
    <...>
    WinHttpCloseHandle(internet);
    }
  return true;
}

Опять же, на мой взгляд, весь этот код из лестниц выглядит криво, и пример с goto выглядит симпатичнее:

 #include <winhttp.h>

bool SendRequest(<...>){
  bool result = false;
  HINTERNET internet = NULL;
  HINTERNET connect = NULL;
  HINTERNET request = NULL;

  internet = WinHttpOpen(<...>);
  if (!internet){
  goto CLOSE_HANDLES_AND_RET;
  }
  <...>
  connect = WinHttpConnect(<...>);
  if(!connect){
  goto CLOSE_HANDLES_AND_RET;
  }
  <...>
  request = WinHttpOpenRequest<...>);	
  if(!request){
  goto CLOSE_HANDLES_AND_RET;
  }
  <...>
  result = true;

  CLOSE_HANDLES_AND_RET:
  if(internet){
  WinHttpCloseHandle(internet);
  }
  if(internet){
  WinHttpCloseHandle(connect);
  }
  if(internet){
  WinHttpCloseHandle(request);
  }

  return result;
}

Я думаю, второй фрагмент проще. При ошибке в промежуточных действиях мы должны освободить ресурсы, не выполнять других промежуточных действий и вернуть false. В первом примере сделать это гораздо сложнее.

Более сложный пример кода можно найти на MSDN

Почему вредно

Если коротко, то при злоупотреблении goto ваш код становится запутанным и сложным для восприятия, а отследить пути выполнения программы становится всё сложнее

Глобальные переменные

Глобальные переменные в C и C++ объявляются вне тела какой-либо функции и видны всей программе. Что касается их применения, лично мне сразу же на ум приходит UEFI и его структуры данных EFI_SYSTEM_TABLE, EFI_BOOT_SERVICES, EFI_RUNTIME_SERVICES. Данные структуры предоставляют приложению интерфейс для работы с аппаратной частью. Указатель на структуру EFI_SYSTEM_TABLE (она содержит указатели на две другие структуры) передаётся в качестве параметра в точку входа программы.

#include <Uefi.h>
    	
EFI_STYSTEM_TABLE *gST = NULL;
EFI_BOOT_SERVICES *gBS = NULL;
    
EFI_STATUS EFIAPI EntryPoint(HANDLE image_handle, EFI_SYSTEM_TABLE *system_table){
  gST = system_table;
  gBS = gST->BootServices;
   
  <Основной код>
  return EFI_SUCCESS;
}

Как-то так выглядит код точки выхода программы под UEFI. Указатели на структуры сохраняются для дальнейшего использования.

Когда может быть полезно

Польза от применения глобальных переменных заключается в простоте доступа к ним. Продолжая пример с UEFI: управление памятью происходит с помощью функций структуры EFI_BOOT_SERVICES (например, AllocatePool и FreePool). Итак, вы написали свои обёртки для функций работы с памятью, написали функции для создания различных необходимых в процессе работы объектов; функции, которые инкапсулируют процессы подготовки и освобождения ресурсов. В результате получаем что-то подобное:

#include <Uefi.h>
	
VOID* SimpleAlloc(EFI_BOOT_SERVICES* boot_serv, size){
	return boot_serv->AllocatePool(size, <...>);
}

EFI_STATUS AllocateNecessaryStructs(EFI_BOOT_SERVICES* boot_serv, <...>){
	VOID* buffer = SimpleAlloc(boot_serv, 123);
	<...>
	return EFI_SUCCESS;
}

EFI_STATUS Prepare(EFI_BOOT_SERVICES* boot_serv, <...>){
	EFI_STATUS last_status = AllocateNecessaryStructs(boot_serv, <...> );		
	<...>
	return EFI_SUCCESS;
}

EFI_STATUS EFIAPI EntryPoint(HANDLE image_handle, EFI_SYSTEM_TABLE *system_table){
	EFI_STATUS last_status = Prepare(system_table->BootServices, <...> );
	<...>
   	return EFI_SUCCESS;
}

Как можно заметить, мы вынуждены тянуть указатель на EFI_BOOT_SERVICES через все функции, в которых (и во вложенных функциях которых) будет происходить работа с памятью. Это не совсем правильно, потому что захламляет стек и приводит к постоянному копированию значения указателя. А что, если нам понадобятся и две другие структуры?

При хранении указателя на EFI_BOOT_SERVICES вы лишены этих проблем и приведённый выше код становится проще

#include <Uefi.h>
	
EFI_STYSTEM_TABLE *gST = NULL;
EFI_BOOT_SERVICES *gBS = NULL;

VOID* SimpleAlloc(size){
	return gBS->AllocatePool(size, <...>);
}

EFI_STATUS AllocateNecessaryStructs(<...>){
	VOID* buffer = SimpleAlloc(123);
	<...>
	return EFI_SUCCESS;
}

EFI_STATUS Prepare(<...>){
	EFI_STATUS last_status = AllocateNecessaryStructs(<...> );		
	<...>
	return EFI_SUCCESS;
}

EFI_STATUS EFIAPI EntryPoint(HANDLE image_handle, EFI_SYSTEM_TABLE *system_table){
	gST = system_table;
	gBS = gST->BootServices;
	EFI_STATUS last_status = Prepare(<...> );
	<...>
   	return EFI_SUCCESS;
}

Почему вредно

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

VOID* CustomMalloc(UINT64 BufferSize){
  if(gBS = NULL){
    return NULL;
  }
  
  VOID* Buffer = gBS->AllocatePool(...);
  <...>
}

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

Массив на стеке

Про массив на стеке довольно подробно уже написано в статье PVS-Studio, здесь я хочу остановиться только на примере.

Когда может быть полезно

Когда вы точно знаете размер используемого буфера или его максимально возможный размер. Например, функция WinAPI GetModuleFileName() принимает указатель на массив символов для сохранения результата. Максимальный размер пути в Windows 260 символов. Соответственно, мы можем выделить память на стеке, так как точно знаем максимальный размер.

bool GetProcessName(std::wstring& proc_name) {
  wchar_t process_path[MAX_PATH];
  
  if (!GetModuleFileName(NULL, process_path, MAX_PATH)) {
    return false;
  }
  
  std::wstring process_name = process_path;
  proc_name = process_name.substr(process_name.find_last_of('\\') + 1);

  return true;
}

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

Почему вредно

Подробно о вреде использования массивов на стеке рассказано всё в той же статье PVS-Studio. Я только отмечу, что такой подход приводит к избыточности, поскольку мы почти всегда выделяем буфер с запасом; а переполнение такого массива может привести к изменению её поведения злоумышленником (например, к выполнению shell-кода)

Напишу всё сам

Я не буду приводить примеров кода, но постараюсь рассказать о моем опыте и примере, с которым мне пришлось столкнуться.

В какой-то момент передо мной встала задача из-под UEFI перечислить все подключенные PCI-устройства и узнать, какие из них являются PCI-to-PCI мостами. Казалось бы, можно использовать те протоколы, что предоставляет сам UEFI, вызывать функции интерфейса для получения адреса устройства и его типа и все будет хорошо. Вот только отдельного протокола для работы с мостами в UEFI нет (поправьте, если я ошибаюсь). Всё равно нужно читать из конфигурационного пространства PCI-устройства. К тому же, функция GetLocation() возвращает 3 UINTN (UINT64 в моём случае) для указания положения устройства (номер сегмента я здесь не учитываю), тогда как оно полностью умещается в UINT16 (8 бит на номер шины + 5 бит на номер устройства + 3 бита на номер функции). Добавим сюда процесс перечисления, который требует выделения памяти под хэндлы устройств и её последующего освобождения.

В общем, было принято решение написать самостоятельно. Собственная урезанная реализация заняла 150 строк кода и время на чтение спецификации, так что затраты на разработку были минимальными. Решение получилось менее универсальным, но для поставленной задачи подходило полностью и показало стабильную работу.

Когда может быть полезно

В описанной мной ситуации я вижу следующие плюсы от самописного решения:

Я разобрался в работе технологии, лучше понял тонкости работы
Реализован и используется только необходимый функционал
Добавлен специфичный для задачи функционал, которого не было в стандартном интерфейсе
Я не навредил процессу разработки основного решения и не сорвал никаких сроков

Почему вредно

Проблемы возникают, когда разработчик пытается решить более сложные задачи. Мы живём в прекрасное время, когда до нас уже написали тонны кода, сделали его универсальным и быстрым. Не следует писать свою библиотеку работы со строками просто потому что - вы не имеете того же опыта, что имели разработчики стандартной библиотеки. Вы потратите уйму времени на то, чтобы добиться такой же производительности и обеспечить тот же функционал. При этом не стоит забывать про безопасность. Конечно, в учебных целях вы вольны делать что захотите, но при работе над продуктом задумайтесь, стоят ли того затраты на изучение, разработку, тестирование и документацию собственного решения. Сможет ли оно обеспечить тот же уровень безопасности, как и уже существующие?

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

Заглянуть за пределы массива

Здесь хотелось бы рассказать немного о массивах с длиной 0. Как вы понимаете, для таких массивов любые операции по чтению и записи должны приводить к выходу за пределы массива. Возможно, я натягиваю сову на глобус, но здесь хотелось бы поделиться полезным опытом и рассказать, как такие массивы позволяют легально производить подобные операции:

typedef struct _PACKET{
  uint64_t	sign;
  uint64_t	id;
  uint64_t	answer_len;
  uint8_t	raw_answer[0];
} PACKET;

void* GetCopyOfRawAnswer(uint8_t* full_pack){
  PACK* pack = reinterpret_cast<PACK*>(full_pack);
  if(pack->sign != PACK_SIGN){
    return nullptr;
  }

  uint8_t* raw_answer = new uint8_t[pack->answer_len];
  memcopy(raw_answer, pack->raw_answer, pack->answer_len);
  return raw_answer;
}

Как это работает? При таком объявлении массива в конце структуры компилятор позволяет нам работать с оставшимся данными по имени массива. Почему не uint8_t* raw_answer? Все потому, что тогда те 8 байт данных (или то число, какое используется на данной платформе), которые лежат после поля answer_len будут восприняты как указатель. О значении этого указателя можно только догадываться, как и о том, к чему приведёт его разыменование. Возможность использования массивов с размером 0 зависит от компилятора (точно поддерживаются MSVC и GCC в качестве расширения).

Когда может быть полезно?

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

Почему вредно?

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

typedef struct _pack{
  uint8_t  type;
  uint64_t len;
  uint8_t  sig;
  uint64_t data[0];
} pack;

int main() {
 uint8_t packet[] = {
  0x1,
  0x2, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
  0xAD,
  0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8,
  0x9, 0xA, 0xB, 0xC, 0xD, 0xE, 0xF, 0x10
  };

  pack* p = (pack*)packet;
}

Как думаете, что содержится в полях данной структуры? Зависит от ситуации, но на моём стенде результат следующий:

Packet info:
type: 1
len: 60504030201ad00
sig: 7
data[0]: cccccccccccc100f
data[1]: cccccccccccccccc

Из-за выравнивания полей структур данные утеряны. Для исправления ситуации необходимо упаковать структуру:

#pragma pack(push, 1)
typedef struct _pack{
  uint8_t  type;
  uint64_t len;
  uint8_t  sig;
  uint64_t data[0];
}pack;
#pragma pack(pop)

Это позволит нам получить желаемый результат:

Packet info:
type: 1
len: 2
sig: ad
data[0]: 807060504030201
data[1]: 100f0e0d0c0b0a09

Заключение

Как говорит мой знакомый профессор математики: "Если бы было лучшее решение, других бы не было". Не стоит сразу клеймить решение, если вы видели его в статьях с названием а-ля "10 антипаттернов <...>". Возможно, оно продиктовано вескими причинами, о которых вы не знаете.

Будьте гибкими, не создавайте себе стереотипов. Думайте о том, как сделать лучше, но не забывайте сначала сделать просто хорошо.