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

FeatureRequest: Add opt-in possibility to zeroed-out buffers #267

Closed
lklein53 opened this issue Feb 6, 2023 · 10 comments · Fixed by #270
Closed

FeatureRequest: Add opt-in possibility to zeroed-out buffers #267

lklein53 opened this issue Feb 6, 2023 · 10 comments · Fixed by #270
Milestone

Comments

@lklein53
Copy link
Contributor

lklein53 commented Feb 6, 2023

Hi together,

Would it be possible to extend the RecyclableMemoryStreamManager by a configuration option to zero-out buffers before returning them to the pool?
I understand that for performance reasons, the buffers are not ever pre-initialized or zeroed-out by default, but i think it would be nice to have the option to configure it if one is willing to take the performance hit.

I could create a PR for this if you think it would be a useful addition.

Best regards,
Lars

@benmwatson
Copy link
Member

I'm not opposed to this. Go for it.

@benmwatson
Copy link
Member

What about adding API to zero out the stream, so the caller is responsible. That would avoid having to add an if check that most people don't need.

@grbell-ms
Copy link
Member

Do you want to add zeroing to prevent accidental data leaks or so that new streams only contain zeros? In the latter case, note that new blocks are allocated using GC.AllocateUninitializedArray which can return arrays with non-zero content.

@lklein53
Copy link
Contributor Author

lklein53 commented Feb 6, 2023

I want to add zeroing to prevent accidental data leaks.

@lklein53
Copy link
Contributor Author

lklein53 commented Feb 6, 2023

What about adding API to zero out the stream, so the caller is responsible. That would avoid having to add an if check that most people don't need.

With the API approach one would have the risk of accidental data leak in case the caller forgets to explicitly zero out memory. Also the combination with `AggressiveBufferReturn`` seems tricky.
I see the point of not adding an if check that most people don't need.
Not sure how to proceed as i think the API approach is already possible by wrapping the stream and calling

CryptographicOperations.ZeroMemory(wrappedStream.GetBuffer());

@grbell-ms
Copy link
Member

Calling GetBuffer() won't return the encapsulated buffers if there are more than one. Do something like this instead:

var buffers = someStream.GetReadOnlySequence();
foreach (var buffer in buffers)
{
    CryptographicOperations.ZeroMemory(buffer.Span);
}

@lklein53
Copy link
Contributor Author

lklein53 commented Feb 6, 2023

This won't work as the buffers are read-only. buffer.Span will return a ReadOnlySpan

@grbell-ms
Copy link
Member

grbell-ms commented Feb 6, 2023

D'oh, you're right. It'd have to be something like this:

var buffers = someStream.GetReadOnlySequence();
foreach (var buffer in buffers)
{
    ref var bytes = ref MemoryMarshal.GetReference(buffer.Span);
    var span = MemoryMarshal.CreateSpan(ref bytes, buffer.Length);
    CryptographicOperations.ZeroMemory(span);
}

But that still won't handle the large buffer if GetBuffer() was ever called during the lifetime of the stream.

@lklein53
Copy link
Contributor Author

lklein53 commented Feb 7, 2023

Would it be okay then to zero out the buffers (only if configured) before returning them to the pool ?
This would require an additional if check

@benmwatson
Copy link
Member

Would it be okay then to zero out the buffers (only if configured) before returning them to the pool ? This would require an additional if check

I think this fine. Let's make this a 3.0 feature.

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