Delivery handlers can be invoked with deallocated or modified memory contents

[Anarh2404]

In #732 was introduced pooled message body ( 8bf9308 )
But the command is not invoked immediatly, just shedulled. So when the event handler is invoked, the command object can already be disposed and its memory region zeroed out/overwritten.

Test (@michaelklishin I made it single threaded) https://github.com/Anarh2404/rabbitmq-dotnet-client/blob/unexpected-frame-repro/projects/Unit/TestFloodPublishing.cs#L89

[michaelklishin]

I don't understand what the problem really is and what options do we have. @stebet do you?

[stebet]

I'll take a look, this shouldn't happen, as we're not supposed to dispose the pooled memory until the work has been completed.

[michaelklishin]

Alternatively, we can yield a copy to the delivery handler. We would lose some of the efficiency gains but safety is a very important factor for the users of this client. In fact, it remains to be seen how many would be tripped by the new copy-or-use requirements in 6.0.

[stebet]

Alternatively, we can yield a copy to the delivery handler. We would lose some of the efficiency gains but safety is a very important factor for the users of this client. In fact, it remains to be seen how many would be tripped by the new copy-or-use requirements in 6.0.

My thoughts exactly. We still shouldn't lose that much efficiency since that can be pooled as well. The overhead is just memory copies taking place, but allocations shouldn't be affected. I can make that change and submit it.

[bording]

My thoughts exactly. We still shouldn't lose that much efficiency since that can be pooled as well. The overhead is just memory copies taking place, but allocations shouldn't be affected. I can make that change and submit it.

Before making more changes, could we try to figure out why the existing code has a problem since @stebet you said you didn't think it was possible.

[stebet]

@bording See my PR with the possible fix. It's due to the deliveries being put on separate worker pools, so after enqueuing the work. See line 58 in Session.cs. After handing the command off to whomever should handle it (in this case a Consumer that later enqueues the work asynchronously), the command, along with the pooled memory it contains is disposed, and might be zeroed out/reused. The PR makes the Consumer Dispatcher copy it before dispatching, and the changes put it on the workers to dispose the new pool once they have finished processing them.

It adds memory copies and churn a bit, but should not increase allocations since they are also utilizing pooled memory.

[stebet]

Eventually, we should just always do this in an async fashion, make message consumption async by default, and deprecate anything else (it's still possible to have Eventing and non-async on top of that). Then this can be done more reliably and we can simply avoid having any special-made worker/pooling constructs. I can take a look at that soonish.

[michaelklishin]

@stebet hey, we can do that as soon as 7.0 :)

[lukebakken]

Fixed by #804. Thanks everyone!