Skip to content

Conversation

@vcsjones
Copy link
Member

This adds a new callback-based mechanism for AsnWriter, and uses it where appropriate through the rest of the libraries.

Closes #75759

@ghost
Copy link

ghost commented Aug 20, 2024

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
@ghost
Copy link

ghost commented Aug 20, 2024

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@vcsjones
Copy link
Member Author

@bartonjs thought: someone could write, reset, clear, etc. the AsnWriter in its own Encode callback either by capturing it or passing itself as TState. Do we want to block that?

Basically, change our encode impl to

try
{
    _encoding = true;
    ReadOnlySpan<byte> encoded = EncodeAsSpan();
    return encodeCallback(encoded);
}
finally
{
    _encoding = false;
}

And check that flag is false in all writing functions.

Is that useful or am I over thinking it?

@bartonjs
Copy link
Member

Any time that we're willing to release the data from Encode, it's already at a place it will never change (except Reset()). Any other calls would just be appending new data beyond where the span is pointing, so it wouldn't have any major surprises.

writer.Reset();
writer.WriteNull();
writer.Encode((writer, span) => writer.WriteEncodedValue(span));
writer.Encode();// now returns [ 0x05, 0x00, 0x05, 0x00 ]

I can't come up with a case where it would result in overwriting existing data. The biggest surprise I can come up with is if the callback does PushSequence/SetOf/OctetString, since it would move the object from "encodable" to "not encodable".

If you want to block it, that's fine, but instead of reading _encoding in every Write function you could probably chokepoint it in EnsureWriteCapacity, since every write function calls that first.

@vcsjones
Copy link
Member Author

it's already at a place it will never change

If the write causes the buffer to resize, the span will be pointing at the old buffer, which has been zeroed.

byte[]? oldBytes = _buffer;
Array.Resize(ref _buffer, BlockSize * blocks);
oldBytes?.AsSpan(0, _offset).Clear();

@bartonjs
Copy link
Member

Ah, so in that case

writer.Encode(
    (writer, span) =>
    {
        writer.WriteNull();
        writer.WriteEncodedObject(span);
    });

would work 99% of the time, but would try writing all zeros (which should fail unless it happens to be 00 00) if the WriteNull forced a resize.

Okay, yeah, may as well block it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: AsnWriter Encode with callback

2 participants