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

BasicGetResult body memory safety in 6.x #994

Closed
Pliner opened this issue Dec 30, 2020 · 11 comments
Closed

BasicGetResult body memory safety in 6.x #994

Pliner opened this issue Dec 30, 2020 · 11 comments

Comments

@Pliner
Copy link
Contributor

Pliner commented Dec 30, 2020

Hi,

It looks I've accidentally found an amusing bug. Let me start from this example(by the way, I'm using v6.2.1):

using System;
using System.Collections.Generic;
using System.Threading;
using RabbitMQ.Client;


namespace DotNetExperiments
{
    public static class Program
    {
        private static readonly ReadOnlyMemory<byte> SentBody = new byte[] {42, 42, 42, 42, 42};
        
        public static void Main()
        {
            // Boostrap stuff
            var connectionFactory = new ConnectionFactory
            {
                Uri = new Uri("amqps://apwlvbny:[email protected]/apwlvbny")
            };
            var connection = connectionFactory.CreateConnection();
            var model = connection.CreateModel();
            var queue = Guid.NewGuid().ToString("N");
            model.QueueDeclare(queue, false, false);

            // Let's store all received bodies
            var receivedBodies = new List<ReadOnlyMemory<byte>>();
            while (true)
            {
                Thread.Sleep(100);

                // Then every iteration we recheck all received bodies: they should match with sentBody 
                foreach (var receivedBody in receivedBodies) CheckReceivedBody(receivedBody);

                model.BasicPublish("", queue, true, null, SentBody);
                var getResult = model.BasicGet(queue, true);
                if (getResult == null)
                    continue;

                // Store received body
                receivedBodies.Add(getResult.Body);
            }
        }

        private static void CheckReceivedBody(ReadOnlyMemory<byte> receivedBody)
        {
            if (receivedBody.Span.SequenceEqual(SentBody.Span))
                return;
            
            Console.WriteLine($"Mismatch: {string.Join(",", receivedBody.Span.ToArray())} != {string.Join(",", SentBody.Span.ToArray())}");
        }
    }
}

After a couple of seconds mismatches have been found:

Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42
Mismatch: 8,0,0,0,0 != 42,42,42,42,42

An array under BasicGetResult.Body was reused.

The reason of it might be the following: after a completion of BasicGet RPC continuation, a command is disposed and an array under BasicGetResult.Body is returned to ArrayPool. This makes usage of BasicGetResult.Body unpredictable at least(EasyNetQ/EasyNetQ#1153) because it is unsafe to use BasicGetResult.Body right after model.BasicGet returns it.

It looks like the only fix is to copy a body a bit before a completion of a continuation.

Could you please confirm the issue?

@michaelklishin
Copy link
Member

This is a 6.x client requirement for regular consumers. We could return a copy for basic.get responses since basic.get by definition means you throw efficiency out the window (there is no way to do polling efficiently).

@michaelklishin michaelklishin changed the title BasicGetResult Body is corrupted BasicGetResult body memory safety in 6.x Dec 30, 2020
@Pliner
Copy link
Contributor Author

Pliner commented Dec 30, 2020

This is a 6.x client requirement for regular consumers.

Yep, I'm aware of this and it's fine for consumers which receive messages via push from broker.

I would like to stress that it looks like there is no way to safely use basic.get response, because the buffer(I mean a byte array which is used by ReadOnlyMemory<byte> in the response) had been already returned to the pool, before we had a chance to read the response.

@bollhals
Copy link
Contributor

For 6.x it's a valid issue from what I can see which got fixed by #919 which references #918.

@Pliner
Copy link
Contributor Author

Pliner commented Dec 30, 2020

@bollhals Have #919 had a chance to be backported to 6.x? Or #995 is preferred way to fix?

@bollhals
Copy link
Contributor

We'd have to investigate how much changed already between 6.x and #919, in general though, it should be possible. One thing though is that BasicGetResult would have to add IDisposable, which is a breaking change, so if that is approved, I think this should be doable.
(Your fix for sure is a quick fix that does the trick as well)

@michaelklishin
Copy link
Member

So do we want to accept #995 (thank you for taking the time to contribute, @Pliner!)? It seems reasonable to me because polling and efficiency really do not go hand in hand :) @stebet @bollhals @bording

@micdenny
Copy link
Contributor

micdenny commented Dec 31, 2020

I would accept #995 me too, because it's easier, and then wait the better fix coming from 8.x

@bollhals
Copy link
Contributor

Fine by me 👍🏼

@TheMulti0
Copy link

Hi, I'm encountering this issue too.
Is there any information known about when will the fix be released on Nuget?
Thanks!

@TheMulti0
Copy link

Apparently my issue was different - it had been passing the body between different threads which resulted in the data getting garbaged.
Thanks!

@Pliner
Copy link
Contributor Author

Pliner commented Jun 28, 2022

The fix was released as part of 6.2.2.

@Pliner Pliner closed this as completed Jun 28, 2022
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

No branches or pull requests

5 participants