Враждебный pack с count=65536 и rec_size=32768. На -O0 получаю "pack too large". На -O2 вместо этого получаю "out of memory". Исходник один и тот же, и я правда не понимаю, как так. -O2 неправильно компилирует, или malloc делает что-то странное?
int total = count * rec_size;
if (total < 0) {
LOG("pack too large");
fclose(f);
return -1;
}
out->data = malloc(total);Hostile pack with count=65536 and rec_size=32768. At -O0 I get "pack too large". At -O2 I get "out of memory" instead. Same source, and I really don't see how. Is -O2 miscompiling it, or is malloc doing something odd?
int total = count * rec_size;
if (total < 0) {
LOG("pack too large");
fclose(f);
return -1;
}
out->data = malloc(total);65536 * 32768 — это 2^31, что не помещается в знаковый int. Это неопределённое поведение, так что ни одна из веток не "правильная". На -O0 оно случайно заворачивается в INT_MIN, и ваша проверка срабатывает. На -O2 компилятор может исходить из того, что переполнения нет, и выкинуть проверку < 0. malloc тут ни при чём. Проверяйте до умножения, например count > INT_MAX / rec_size, или используйте size_t.
65536 * 32768 is 2^31, which doesn't fit in a signed int. That's undefined behavior, so neither branch is "correct". At -O0 it happens to wrap to INT_MIN and your check fires. At -O2 the compiler may assume no overflow and drop the < 0 test. malloc isn't odd. Check before multiplying, e.g. count > INT_MAX / rec_size, or use size_t.
Ха, классика. Причину уже назвал e.lund. Добавлю часть, которая объясняет ваше сообщение "out of memory".
Если компилятор выбрасывает проверку < 0, в total всё равно лежит завёрнутое значение, INT_MIN. Затем malloc(total) преобразует его в size_t. На 64-битной машине это становится 0xFFFFFFFF80000000, то есть около 16 эксабайт. malloc отказывает, и срабатывает ваша OOM-ветка. Так что malloc не странный. Просто у него просят что-то абсурдное. (Кстати, поэтому у тех, кто твердит "просто проверяйте на NULL", тут возникает ложное чувство безопасности.)
Мой вопрос про объявления. Какие типы у count и rec_size? Если они читаются из файла как uint32_t, то умножение происходит в беззнаковой арифметике и законно заворачивается. Тогда присваивание в int total — implementation-defined, а не UB. Это сильно меняет картину.
Пробовали -fsanitize=undefined на -O2? Он должен прямо пометить умножение. И приводит ли -fwrapv оба билда снова к одинаковому поведению?
Ha, this is a classic. e.lund has the cause. I'll add the part that explains your "out of memory" message.
If the compiler drops the < 0 test, total still holds the wrapped value, INT_MIN. Then malloc(total) converts it to size_t. On a 64-bit box that becomes 0xFFFFFFFF80000000, which is about 16 exabytes. malloc says no, and your OOM branch fires. So malloc isn't odd. It's just being asked for something absurd. (Side note: that's also why the "just check for NULL" crowd gets a false sense of safety here.)
My question is about the declarations. What types are count and rec_size? If they're read from the file as uint32_t, the multiply happens in unsigned arithmetic and wraps legally. Then the int total assignment is implementation-defined, not UB. That would change the story a lot.
Have you tried -fsanitize=undefined at -O2? It should flag the multiply directly. And does -fwrapv make the two builds agree again?
Проверка < 0 выполняется уже после того, как переполнение произошло, так что я бы не пытался её спасать. Проверяйте значения из заголовка до умножения и держите результат в size_t.
size_t total;
if (rec_size == 0 ||
__builtin_mul_overflow((size_t)count, (size_t)rec_size, &total) ||
total > max_pack_bytes) {
LOG("pack too large");
fclose(f);
return -1;
}С size_t 2^31 — вполне допустимый размер, так что ваш враждебный pack теперь попросит у malloc 2 GiB. Одной проверки переполнения для этого недостаточно. Я бы ограничил max_pack_bytes размером файла или бюджетом вашего загрузчика, чтобы заголовок не мог заявлять больше, чем есть в файле.
Билтин описан здесь: https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html. Он есть и в GCC, и в clang. Если собираете MSVC, используйте вариант count > SIZE_MAX / rec_size.
The < 0 test runs after the overflow has already happened, so I wouldn't try to rescue it. Validate the header values before you multiply, and keep the result in size_t.
size_t total;
if (rec_size == 0 ||
__builtin_mul_overflow((size_t)count, (size_t)rec_size, &total) ||
total > max_pack_bytes) {
LOG("pack too large");
fclose(f);
return -1;
}With size_t, 2^31 is a perfectly valid size, so your hostile pack would now ask malloc for 2 GiB. Overflow checking alone doesn't catch that. I'd cap max_pack_bytes at the file size, or whatever your loader budget is, so a header can't claim more than the file holds.
The builtin is documented here: https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html. GCC and clang have it. If you build with MSVC, use the count > SIZE_MAX / rec_size form instead.
Это не ошибка компилятора. Знаковое переполнение — UB, поэтому компилятор вправе удалить проверку < 0. Посмотрите asm через gcc -O2 -S, и вы увидите, что сравнения там вообще нет.
Остаётся 32-битный imul. Результат 0x80000000. Затем movslq расширяет его со знаком для аргумента malloc, и получается 0xFFFFFFFF80000000. malloc на этом падает, и вы попадаете в OOM-ветку.
На -O0 сравнение всё ещё генерируется и видит знаковый бит, поэтому получаете "pack too large".
Одной починки проверки недостаточно. Даже с size_t 2^31 — это всего 256 MB, так что malloc спокойно вам это выдаст. Враждебный заголовок может просто запросить меньше, чем граница переполнения. Сравнивайте с реальной длиной файла до выделения памяти. И в итоге ещё ограничьте count.
Not a miscompile. Signed overflow is UB, so the compiler may delete the < 0 test. Look at the asm with gcc -O2 -S and you will see no compare at all.
What is left is a 32-bit imul. The result is 0x80000000. Then movslq sign-extends it for the malloc argument, which gives 0xFFFFFFFF80000000. malloc fails on that, so you land in the OOM branch.
At -O0 the compare is still emitted and sees the sign bit, so you get "pack too large".
Fixing the check is not enough. Even with size_t, 2^31 is only 256 MB, so malloc will happily give it to you. A hostile header can just ask for less than the wrap point. Compare against the real file length before you allocate. Eventually also cap count.
Хорошо подмечено про movslq. Именно на этом шаге INT_MIN превращается в тот запрос на 16 эксабайт, о котором я говорил выше.
Одна придирка. 2^31 байт — это 2 GiB, а не 256 MB. drawer6 был прав. Это только усиливает ваш аргумент. В Linux с overcommit по умолчанию malloc на 2 GiB обычно проходит мгновенно, потому что страницы ещё не тронуты. Так что "исправленная" версия с size_t не падает чисто. Она возвращает указатель, а проблемы начинаются позже, когда чтение или memset подгружают страницы. (Спросите, откуда я знаю. Однажды обработчик загрузок у клиента делал ровно это, и OOM killer прибил не тот процесс.)
Так что да, сначала сравнивайте с реальной длиной файла. Размер даёт fstat. Вычтите байты заголовка, которые уже прочитаны, и проверьте count * rec_size против оставшегося, в форме, безопасной от переполнения.
Кроме того, если кто-то собирает это под 32 бита, size_t там 32-битный, и умножение может законно завернуться в беззнаковой арифметике. Проверка переполнения там всё равно нужна. Одна проверка длины файла вас не спасёт, если завёрнутое произведение случайно окажется маленьким.
Good catch on the movslq. That's the exact step where INT_MIN turns into the 16-exabyte request I was waving at earlier.
One nit, though. 2^31 bytes is 2 GiB, not 256 MB. drawer6 had it right. That makes your point stronger. On Linux with default overcommit, a 2 GiB malloc will usually succeed instantly, because the pages aren't touched yet. So the "fixed" size_t version doesn't fail cleanly. It returns a pointer, and the trouble starts later, when the reads or a memset fault the pages in. (Ask me how I know. A client's upload handler once did exactly this, and the OOM killer took out the wrong process.)
So yes, compare against the real file length first. fstat gives you the size. Subtract the header bytes you've already consumed, and check count * rec_size against what's left, using the overflow-safe form.
Also, if anyone builds this for 32-bit, size_t is 32 bits and the multiply can wrap legally in unsigned arithmetic. The overflow check still matters there. The file-length check alone won't save you if the wrapped product happens to be small.
int total = count * rec_size;
У меня сомнение: почему размер в байтах вообще int? Размеры должны быть size_t с момента чтения заголовка. И count с rec_size ведь приходят прямо из файла, нет? Тогда проверка после умножения запоздала by design. Сначала прогоните UBSan на -O2.
int total = count * rec_size;
I have a doubt: why is a byte size an int at all? Sizes should be size_t from the moment you read the header. And count and rec_size come straight from the file, no? Then checking after the multiply is late by design. Run UBSan at -O2 first.
Может ли быть, что race detector здесь ошибается, или я неправильно понимаю, что мне даёт ack? Это pushgate, небольшой сервис…
Написал небольшой чекер на Node (чистый JS, без сборки): он переигрывает сохранённые доставки вебхуков и проверяет HMAC-SHA25…
Пишу небольшой хранитель учётных данных на C (WSL, Neovim, ничего особенного). vault_release() должна затирать секрет, а пото…
