postgrespro/pg_query_state

Используется memcpy() для строк, не добавляется '\0'

ekzolol opened this issue · 2 comments

Статический анализатор PVS Studio выдал такие ошибки:
contrib/pg_query_state/signal_handler.c 125 err V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null.
contrib/pg_query_state/signal_handler.c 129 err V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null.

Вот, собственно, тот кусок, на который он ругается:

static void
serialize_stack_frame(char **dest, stack_frame *qs_frame)
{
	SET_VARSIZE(*dest, strlen(qs_frame->query) + VARHDRSZ);
	memcpy(VARDATA(*dest), qs_frame->query, strlen(qs_frame->query));
	*dest += INTALIGN(VARSIZE(*dest));

	SET_VARSIZE(*dest, strlen(qs_frame->plan) + VARHDRSZ);
	memcpy(VARDATA(*dest), qs_frame->plan, strlen(qs_frame->plan));
	*dest += INTALIGN(VARSIZE(*dest));
}

Я, если честно, так сразу не понимаю, false positive это или нет. Зависит от контекста.

Например, если данные из dest где-то печатаются, то в конце должен быть '\0', иначе это undefined behaviour. И если под dest изначально выделен фиксированный участок памяти, а то, что мы туда копируем, может быть неограниченно длинным, то неплохо было бы проверять длину и убеждаться, что мы не выйдем за этот фиксированный участок памяти.

Привет, @ekzolol

PVS Studio - это хорошо, наверняка предупреждений было еще больше. Возможно, вы нашли какой-то баг и решили проверить код?

Я не являюсь автором этого расширения, но беглый просмотр кода свидетельствует не в пользу ваших замечаний:

  • Во-первых, длина заведомо известна, и ее вычисляет функция serialized_stack_length().
  • Во-вторых, перед каждой строкой пишется ее длина (SET_VARSIZE()).

Привет @ekzolol ! Спасибо за проверку исходного кода расширения анализатором PVS Studio.

Как написал @funbringer , '\0' символ на конце не нужен, фактическая длина строки сохраняется перед символьным массивом функцией SET_VARSIZE(). Строка запроса и дерево плана хранятся в виде встроенного типа text, который определён следующим образом:

typedef struct varlena text;

struct varlena
{
	char		vl_len_[4];		/* Do not touch this field directly! */
	char		vl_dat[FLEXIBLE_ARRAY_MEMBER];	/* Data content is here */
};

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

Что касается аллокации памяти под последовательность пар <тело запроса, план запроса>, то весь размер необходимой памяти изначально вычисляется через функцию serialized_stack_length() и динамически выделяется во фрагменте функции SendQueryState():

int		         msglen = sizeof(shm_mq_msg) + serialized_stack_length(qs_stack);
shm_mq_msg		*msg = palloc(msglen);

На мой взгляд, PVS Studio сработал из-за наличия комбинации вызовов memcpy/strlen функций, предположив, что копируются строки в их классическом представлении.