Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Корректность теста PopBadArgs #26

Open
Mako-D opened this issue Nov 26, 2023 · 14 comments · May be fixed by #29
Open

Корректность теста PopBadArgs #26

Mako-D opened this issue Nov 26, 2023 · 14 comments · May be fixed by #29
Assignees

Comments

@Mako-D
Copy link
Collaborator

Mako-D commented Nov 26, 2023

В данном тесте предлагается сделать вложение в стек массива из 5 элементов

const size_t size = 5;
const int data_in[size] = {1};
stack_push(stack, &data_in, sizeof(data_in)); // В стеке [1,0,0,0,0]

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

int data_out[size - 1] = {0};
stack_pop(stack, data_out, sizeof(data_out)); // ???

Тестирующая программа ожидает получить на выходе четыре 0,

EXPECT_THAT(data_out, ::testing::Each(0)); // Программа ожидает на выходе 0,0,0,0
// То есть программа ожидает, что мы начнем читать массив из стека "справа-налево" [1,0,0,0,0]<-
// Хотя добавлялись не элементы массива поочередно, а сразу весь массив
// То есть именно массив у нас является элементом стека

Но это значит, что программа ожидает поэлементного добавления массива в стек, что-то вроде

stack_push(stack, &data_in[0], sizeof(data_in[0]));
stack_push(stack, &data_in[1], sizeof(data_in[1]));
// ...
stack_push(stack, &data_in[4], sizeof(data_in[4]));

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

@czertyaka
Copy link
Owner

czertyaka commented Nov 28, 2023

Но это значит, что программа ожидает поэлементного добавления элементов массива в стек, что-то вроде

Не совсем. В строках

const size_t size = 5;
const int data_in[size] = {1};
stack_push(stack, &data_in, sizeof(data_in));

В стеке в итоге оказывается не 5 элементов {1, 0, 0, 0, 0}, а один элемент размера 5 * sizeof(int). Требований работы с массивами в ТЗ нет, потому что библиотека не знает ничего о принимаемыз данных. Для нее данные -- это просто указатель на участок и его размер. Там может быть один int, структура, неинициализированный участок или массив.

@czertyaka
Copy link
Owner

И еще уточню на всякий случай: рассматриваемый нами тест написан для проверки ситуации, в которой пользователь передал неверные аргументы в функцию stack_pop. В данном случае, он положил в стек элемент размером 5* sizeof(int), а затем просит вытащить его и положить в буфер недостаточного размера 5* sizeof(int).

Поэтому в этой строке

EXPECT_EQ(stack_pop(stack, data_out, sizeof(data_out)), 0u);

ожидается, что stack_pop вернет 0 (как бы говоря, я положила 0 байт в твой буфер, так как места в нем недостаточно); а в строке

EXPECT_THAT(data_out, ::testing::Each(0));

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

@czertyaka
Copy link
Owner

Ваш запрос обратил мое внимание на другую проблему в тестах. Я на самом деле хотел, чтобы весь массив data_in был инициализирован -1, а по невнимательности написал по аналогии с = {0};. Это поправлю сейчас.

@Mako-D
Copy link
Collaborator Author

Mako-D commented Nov 28, 2023

Спасибо!
Забыл, что в случае такого объявления массива

const int data_in[size] = {1};
stack_push(stack, &data_in, sizeof(data_in));

в нем будет лежать не [1,0,0,0,0], а в [1,1,1,1,1], потому не совсем корректно понял сам тест.

@czertyaka
Copy link
Owner

Спасибо! Забыл, что в случае такого объявления массива

const int data_in[size] = {1};
stack_push(stack, &data_in, sizeof(data_in));

в нем будет лежать не [1,0,0,0,0], а в [1,1,1,1,1], потому не совсем корректно понял сам тест.

Тут вы как раз полностью правы были)

@czertyaka czertyaka linked a pull request Nov 28, 2023 that will close this issue
@czertyaka
Copy link
Owner

Я вам инвайт коллаборатора направил, если хотите добавлю в ревьюверы пулл-реквеста с исправлением теста

@czertyaka czertyaka self-assigned this Nov 28, 2023
@Mako-D
Copy link
Collaborator Author

Mako-D commented Nov 28, 2023

В данном случае, он положил в стек элемент размером 5* sizeof(int), а затем просит вытащить его и положить в буфер недостаточного размера 5* sizeof(int).

Ответственность за корректность "вытаскивания" элементов из стека лежит не на пользователе?

@Mako-D
Copy link
Collaborator Author

Mako-D commented Nov 28, 2023

Я вам инвайт коллаборатора направил, если хотите добавлю в ревьюверы пулл-реквеста с исправлением теста

Да, буду за

@czertyaka
Copy link
Owner

czertyaka commented Nov 28, 2023

В данном случае, он положил в стек элемент размером 5* sizeof(int), а затем просит вытащить его и положить в буфер недостаточного размера 5* sizeof(int).

Ответственность за корректность "вытаскивания" элементов из стека лежит не на пользователе?

На пользователе. Он должен представить буфер, с размером не меньшим, чем размер верхнего элемента в стеке. Это следствие того, что данные в буфере не типизированы.

@Mako-D
Copy link
Collaborator Author

Mako-D commented Nov 28, 2023

Разве это тогда не проблема пользователя, что он предоставил размер массива меньший (из 4 * size(int)), чем размер элемента стека (5 * size(int))?

@czertyaka
Copy link
Owner

Разве это тогда не проблема пользователя, что он предоставил размер массива меньший (из 4 * size(int)), чем размер элемента стека (5 * size(int))?

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

С другой стороны, если пользователь создал буфер размера 4 байта, а сам передал в stack_pop аргумент size == 8, то от такой ситуации никак не спасешься. Библиотека не гарантирует, что программа продолжить работать корректно после такого вызова.

@Mako-D
Copy link
Collaborator Author

Mako-D commented Nov 29, 2023

Может есть смысл в ТЗ отдельно добавить требование проверки совпадения размеров полей, которые подает пользователь с размером элемента в стеке? Что-то вроде "стек должен обеспечивать корректный вывод данных" или "библиотека должна гарантировать соответствие размера выводимых из стека данных и поля пользователя". То, что данные не типизированные ещё не означает, что библиотека должна заботиться о пользователе, ограничивая пользователя, если он хочет вытащить данные в поле некорректного размера.

@czertyaka
Copy link
Owner

czertyaka commented Nov 29, 2023

Может есть смысл в ТЗ отдельно добавить требование проверки совпадения размеров полей, которые подает пользователь с размером элемента в стеке? Что-то вроде "стек должен обеспечивать корректный вывод данных" или "библиотека должна гарантировать соответствие размера выводимых из стека данных и поля пользователя".

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

Перечитал ТЗ, возможно этот момент и стоит отдельно обозначить. Что то вроде

  • unsigned int stack_pop(const hstack_t stack, void* data_out, const unsigned int size)
    Brief: Извлечь элемент из стека и записать данные этого элемента в буфер, если соответствующий хэндлеру стек существует и пользователь предоставил буфер достаточного размера.
    Parameters: stack - хэндлер стека; data_out - указатель на буфер для записи данных; size - размер буфера.
    Returns: размер записанных данных в байтах, если соответствующий хэндлеру стек существует и пользователь предоставил буфер достаточного размера, или 0 в противном случае.

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

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

Сравните два варианта:

// push some text
const char* message = "This is important text, do not lose it!";
hstack_t handler = stack_new();
if (handler != -1) {
    stack_push(handler, message, strlen(message));
}

// 1. stack_pop pops data to buffer of insufficient size and silently erases data
char buffer[20] = {0};
unsigned int out = stack_pop(handler, buffer, sizeof(buffer));
// out = 20, *buffer = "This is important te", "xt, do not lose it!" is lost forever

// or...

// 2. stack_pop refuses to pop data to buffer of insufficient size
char* buffer = (char*)calloc(20, 1);
unsigned int out = stack_pop(handler, buffer, 20);
if (out == 0u) {
    buffer = (char*)realloc(buffer, 40);
    out = stack_pop(handler, buffer, 40);
    // out = 39, buffer = "This is important text, do not lose it!"
}

@Mako-D
Copy link
Collaborator Author

Mako-D commented Nov 29, 2023

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

Благодарю за уточнение. Подправлю этот момент в своём домашнем.

Перечитал ТЗ, возможно этот момент и стоит отдельно обозначить.

Выглядит здорово!

Что делать, если пользователь просто ошибся или не знал заранее, какой размер данных на вершине стека? К тому моменту, как он поймет ошибку, данные будут уже утеряны.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants