fix(kafka): fix use-after-free on task cancellation during kafka producer Send - #1299
fix(kafka): fix use-after-free on task cancellation during kafka producer Send#1299disaykin wants to merge 2 commits into
Conversation
30e88de to
82e223b
Compare
|
@apolukhin прошу ревью. проблему с установкой кафки в тестах я починил (там протух урл), но вот что делать с флапающим тестом на постгрю - не знаю: Но это точно не относится к моему фиксу. Также проблема компиляции с rabbitmq на debian12 привнесена не мной/ |
|
Tests in postgres is flap. |
| set -o errexit -o nounset -o pipefail -o posix -x | ||
|
|
||
| KAFKA_VERSION=4.0.1 | ||
| KAFKA_VERSION=4.3.1 |
There was a problem hiding this comment.
Why did you increas it?
There was a problem hiding this comment.
Старая ссылка для установки кафки больше не работает. Заменил на ссылку для скачивания с их официального сайта
There was a problem hiding this comment.
Предлагаете оформить это отдельным запросом на влитие? Или претензия именно к изменению версии? Старая версия у меня вроде бы не захотела скачиваться по новой ссылке...
There was a problem hiding this comment.
Если у них на сайте теперь написано, что надо так, то окей
There was a problem hiding this comment.
тогда предлагаю считать, что все сделано правильно
| set -o errexit -o nounset -o pipefail -o posix -x | ||
|
|
||
| KAFKA_VERSION=4.0.1 | ||
| KAFKA_VERSION=4.3.1 |
There was a problem hiding this comment.
Если у них на сайте теперь написано, что надо так, то окей
| ) const { | ||
| engine::TaskCancellationBlocker blocker; | ||
|
|
||
| if (engine::current_task::IsCancelRequested()) |
There was a problem hiding this comment.
Добавь тест, в котором таска отменяется и происходит use-after-free. Чтобы хотя бы локально убедиться, что бага есть, а после фикса её нет
There was a problem hiding this comment.
Это будет флаки тест. Даже при наличии бага, он будет фейлиться не каждый раз. И фэйлиться будет только под адрес-санитайзером. Я пробовал написать, получается как-то не очень... Попробую откопать свои наработки, может очередная попытка родит что-то удачнее
There was a problem hiding this comment.
О, придумал. Можно выставить большие значения либрдкафка накопления очереди перед сбросом (1000 сообщений, 1 секунда, что раньше наступит). Тогда тест будет не флаки под санитайзером
There was a problem hiding this comment.
Вообще,я имел ввиду, чтобы ты показал нам тест и локально убедился, что он падает без твоего фикса. А с фиксом чинится (и потом его удалить и не мержить). Но если получится даже полноцнный сделать, то шикарно
There was a problem hiding this comment.
написал тесты, если закомментировать блокер, то падает
SUMMARY: AddressSanitizer: heap-use-after-free (/home/disaikin/git/userver/build/kafka/userver-kafka-dbtest+0x48f00c5) (BuildId: df950da811b1ab7c) in memcpy
Shadow bytes around the buggy address:
0x508000004b00: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x508000004b80: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
0x508000004c00: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x508000004c80: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x508000004d00: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
=>0x508000004d80: fa fa fa fa fd fd[fd]fd fd fd fd fd fd fd fd fd
0x508000004e00: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
0x508000004e80: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x508000004f00: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x508000004f80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x508000005000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==2408226==ABORTING
| std::optional<std::uint32_t> partition, | ||
| HeaderViews headers | ||
| ) const { | ||
| engine::TaskCancellationBlocker blocker; |
There was a problem hiding this comment.
Кажется что нужен просто CriticalAsync вместо TaskCancellationBlocker... Или надо перенести TaskCancellationBlocker внутрь Async
There was a problem hiding this comment.
Могу ошибаться, но...
Если засунуть блокер внутрь асинка, то вложенная корутина продолжит выполняться, а внешняя вылетит по исключению из Get(), стек развернется, и память, на которую указывали вьюхи, будет освобождена. Librdkafka попытается отправить в сеть уже освобожденную память.
Если заменить Async на CriticalAsync, то вложенная корутина не отменится, а внешняя сразу же отменится, освободит память, и librdkafka снова попытается отправить в сеть уже освобожденную память.
Тут именно нужно не дать внешней корутине разрушиться, пока librdkafka не закончит трогать память вьюх.
There was a problem hiding this comment.
выкинул лишний код с проверкой отмены задачи на входе в блок, так как вероятность отмены задачи очень низка, а код корутин должен обнаружить отмену задачи при запуске в другом таск-процессоре, если я правильно понимаю его логику.
@apolukhin Я же правильно понимаю, что utils::Async при входе проверит ShouldCancel(), получит false из-за блокера, создаст корутину и отдаст её в другой таск-процессор, который сразу же увидит, что ShouldCancel() равен true и завершит корутину с выбросом исключения?
There was a problem hiding this comment.
был не прав, все совсем не так работает, как я думал. переделал на использование CancellationPoint(), чтобы отмененная задача отменялась на входе в Send() и перед самым выходом из Send(), так как эти места фактически являются местами, где можно безопасно отменить долгую операцию
3ea2658 to
7d693f1
Compare
…ucer Send Fixes a Use-After-Free vulnerability in the asynchronous Kafka Producer API when a task is cancelled before librdkafka flushes its queue. The `kafka::Producer::Send` API accepts parameters as views (`string_view`, `zstring_view`, `HeaderViews`). These views point to data owned by the caller. Inside `Send`, raw pointers from these views are passed down to `librdkafka`, which stores them in its internal asynchronous queues for background delivery. If a client connection drops, a cascade cancellation of tasks occurs. This leads to a premature return from `kafka::Producer::Send`, causing the caller to destroy the original payload memory. However, `librdkafka` still retains these pointers and attempts to access them during subsequent network flushes to the broker, resulting in memory corruption. To fix this, we ensure that `Producer::Send` is blocked from returning prematurely and cannot exit until the background `SendImpl` completes its interaction with `librdkafka`. Co-authored-by: Aleksander Rypalov <rypalov2002@gmail.com>
Old URL returns 404 now
7d693f1 to
003eae8
Compare
Fixes a Use-After-Free vulnerability in the asynchronous Kafka Producer API when a task is cancelled before librdkafka flushes its queue.
The
kafka::Producer::SendAPI accepts parameters as views (string_view,zstring_view,HeaderViews). These views point to data owned by the caller. InsideSend, raw pointers from these views are passed down tolibrdkafka, which stores them in its internal asynchronous queues for background delivery.If a client connection drops, a cascade cancellation of tasks occurs. This leads to a premature return from
kafka::Producer::Send, causing the caller to destroy the original payload memory. However,librdkafkastill retains these pointers and attempts to access them during subsequent network flushes to the broker, resulting in memory corruption.To fix this, we ensure that
Producer::Sendis blocked from returning prematurely and cannot exit until the backgroundSendImplcompletes its interaction withlibrdkafka.Note: by creating a PR or an issue you automatically agree to the CLA. See CONTRIBUTING.md. Feel free to remove this note, the agreement holds.